Page MenuHomePhabricator

phpunit hhvm failure: LuaSandbox: TextLibraryTests[87]: json decode, invalid values (trailing comma)
Closed, DuplicatePublic

Description

This is an issue with HHVM using json-c's default non-strict parsing mode. Upstream bug is https://github.com/facebook/hhvm/issues/5813


I tried running the CI for the build of Wikidata with hhvm instead of zend:
https://integration.wikimedia.org/ci/job/mwext-Wikidata-testextension-hhvm/2/consoleFull
Works on zend: https://integration.wikimedia.org/ci/job/mwext-Wikidata-testextension-zend/359/

There were 2 failures:

  1. LuaSandbox: TextLibraryTests[87]: json decode, invalid values (trailing comma)

Failed asserting that two strings are identical.

  • Expected

+++ Actual
@@ @@
-ERROR: mw.text.jsonDecode: Syntax error
+{
+ {
+ ["x"] = 1,
+ },
+}

/mnt/jenkins-workspace/workspace/mwext-Wikidata-testextension-hhvm/src/extensions/Scribunto/tests/engines/LuaCommon/LuaEngineTestBase.php:252
/mnt/jenkins-workspace/workspace/mwext-Wikidata-testextension-hhvm/src/tests/phpunit/MediaWikiTestCase.php:131

  1. LuaStandalone: TextLibraryTests[87]: json decode, invalid values (trailing comma)

Failed asserting that two strings are identical.

  • Expected

+++ Actual
@@ @@
-ERROR: mw.text.jsonDecode: Syntax error
+{
+ {
+ ["x"] = 1,
+ },
+}

/mnt/jenkins-workspace/workspace/mwext-Wikidata-testextension-hhvm/src/extensions/Scribunto/tests/engines/LuaCommon/LuaEngineTestBase.php:252
/mnt/jenkins-workspace/workspace/mwext-Wikidata-testextension-hhvm/src/tests/phpunit/MediaWikiTestCase.php:131

Event Timeline

JanZerebecki updated the task description. (Show Details)
JanZerebecki raised the priority of this task from to Needs Triage.
JanZerebecki moved this task to Backlog on the MediaWiki-extensions-Scribunto board.
JanZerebecki added a subscriber: JanZerebecki.
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJun 22 2015, 2:08 PM
Anomie added a subscriber: Anomie.Jun 23 2015, 11:28 AM

Bug in HHVM: the json-c library used by hhvm accepts input that's not
actually valid json by default, and HHVM doesn't pass the override flag or
even expose it to PHP code.

Anomie set Security to None.
Anomie updated the task description. (Show Details)Jul 28 2015, 3:09 PM
Anomie added a project: Upstream.
daniel added a subscriber: daniel.Sep 10 2015, 1:19 PM

What's the actual problem here? Are our tests failing, because input that shouldn't be accepted is accepted? Or what?
Do we actually need to fail on a trailing comma? Is it part of our contract to perform strict validation on json input?
What JSON input anyway?

Our tests fail correctly because hhvm accepts input that is invalid json that it should not accept. Suggested action: Fix hhvm.

If the relevant input isn't actually harmful, I'd suggest to relax the tests.
Sure, hhvm should be fixed, and there's a ticket for that. But *we* shouldn't block on hhvm getting fixed.

Are our tests failing, because input that shouldn't be accepted is accepted?

Yes. For example, [1,] is not valid JSON (it's not allowed in either RFC 7159 or ECMA-404), but is accepted by HHVM's json_decode.

Do we actually need to fail on a trailing comma? Is it part of our contract to perform strict validation on json input?

It should at least be consistent on PHP versus HHVM, which currently it's not as shown by this test failure. In general it's best to follow the standards, IMO.

What JSON input anyway?

Whatever JSON input is supplied by the user.

JanZerebecki added a comment.EditedSep 10 2015, 2:13 PM

This tests the json functions we implement for Lua, so yes we need to perform strict validation, as we specified our Lua json_decode function that way.

A compromise would be to mark the relevant test as skipped until this issue is fixed in hhvm.

Change 252508 had a related patch set uploaded (by JanZerebecki):
Temporarily skip a test that fails on HHVM.

https://gerrit.wikimedia.org/r/252508

Restricted Application added a subscriber: StudiesWorld. · View Herald TranscriptNov 11 2015, 8:49 PM

Change 252508 merged by jenkins-bot:
Temporarily skip a test that fails on HHVM.

https://gerrit.wikimedia.org/r/252508

Change 268473 had a related patch set uploaded (by Mattflaschen):
Temporarily skip a test that fails on HHVM.

https://gerrit.wikimedia.org/r/268473

Change 268473 merged by jenkins-bot:
Temporarily skip a test that fails on HHVM.

https://gerrit.wikimedia.org/r/268473

Change 268811 had a related patch set uploaded (by Paladox):
Temporarily skip a test that fails on HHVM.

https://gerrit.wikimedia.org/r/268811

Change 268811 merged by jenkins-bot:
Temporarily skip a test that fails on HHVM.

https://gerrit.wikimedia.org/r/268811

Catrope closed this task as Resolved.Feb 13 2016, 12:29 AM
Catrope added a subscriber: Catrope.
JanZerebecki reopened this task as Open.Feb 13 2016, 9:56 AM

The bug is not fixed, yet. Just the test disabled.

Krinkle moved this task from Backlog to Defect on the HHVM board.Feb 21 2016, 5:57 PM