561985 - Use native JSON functions

Status: RESOLVED WONTFIX

(Tree Management Graveyard :: TBPL, defect, P5)

Description

[Arpad Borsos [:Swatinem]]

Attachment: patch [pushed: comment 1]

PHP had native json support since 5.2 (released in 2006), so use that.

Comment 1

[Arpad Borsos [:Swatinem]]

Comment on attachment 441733 [details] [diff] [review]
patch [pushed: comment 1]

http://hg.mozilla.org/users/mstange_themasta.com/tinderboxpushlog/rev/1dd3a48587eb

Comment 2

[(no longer active)]

There was a reason why I used Services_JSON instead of PHP's native json functions.  Those functions are buggy as hell.  For example, json_encode fails if there is a non-breaking space in the input.

This bug broke TinderboxPushlog Robot completely, and rendered it useless.  I took the liberty of backing it out, and tested that revision locally to make sure that it works again.

http://hg.mozilla.org/users/mstange_themasta.com/tinderboxpushlog/rev/9f45e38f57c9

Markus, could you please deploy this changeset?  Without it, tbplbot can't comment on any bug.

Comment 3

[Arpad Borsos [:Swatinem]]

I took a quick look at PHPs bug tracker and there are indeed quite a few (closed) bugs about encoding errors.

Does the bug also occur with the latest stable release? If so, file a PHP bug.

$data = array(
  "text" => $_GET["comment"]
);

That looks like the only interesting use of json_encode, which does depend on unfiltered user input. That input itself is generated from the tinderbox log, which itself claims to be utf8, but I wouldn't bet on that.

I'm perfectly fine with having this reverted, for now. But if the implementation is "buggy as hell" then get the PHP developers to fix it.

Comment 4

[(no longer active)]

(In reply to comment #3)
> I took a quick look at PHPs bug tracker and there are indeed quite a few
> (closed) bugs about encoding errors.
> 
> Does the bug also occur with the latest stable release? If so, file a PHP bug.
> 
> $data = array(
>   "text" => $_GET["comment"]
> );
> 
> That looks like the only interesting use of json_encode, which does depend on
> unfiltered user input. That input itself is generated from the tinderbox log,
> which itself claims to be utf8, but I wouldn't bet on that.
> 
> I'm perfectly fine with having this reverted, for now. But if the
> implementation is "buggy as hell" then get the PHP developers to fix it.

That code is indeed the one which fails.  In this specific case, the failure is caused by json_encode failing to encode a non-breaking space.

I've filed this bug for the issue:

http://bugs.php.net/bug.php?id=51947

However, even with that fixed, I still won't feel comfortable with using json_encode, because I've had a lot of problems with it, and I don't think that the speed improvement is worth the risk.

Comment 5

[Markus Stange [:mstange]]

(In reply to comment #2)
> This bug broke TinderboxPushlog Robot completely, and rendered it useless.

Huh? Didn't it still work like yesterday? Did I fail to deploy that file earlier?

Comment 6

[Markus Stange [:mstange]]

Deployed the backout, in any case.

Comment 7

[Arpad Borsos [:Swatinem]]

Thank you for filing a php bug. I've commented there on why I think it behaves correctly regarding invalid utf8 chars.

I would suggest fixing the test infrastructure to output correct utf8. Could you please file a bug? You know better than me which part of the tests output this.

Comment 8

[(no longer active)]

(In reply to comment #7)
> Thank you for filing a php bug. I've commented there on why I think it behaves
> correctly regarding invalid utf8 chars.
> 
> I would suggest fixing the test infrastructure to output correct utf8. Could
> you please file a bug? You know better than me which part of the tests output
> this.

That character is actually coming from the browser!

What happens is that we inject a span node with a data-signature attribute containing  , and then we'll retrieve the value and post it to the server, escaped.

This test case also returns 160.
data:text/html;charset=utf-8,<p></p><script>var p=document.querySelector("p");p.innerHTML='&nbsp;';alert(p.textContent.charCodeAt(0))</script>

But this is the decoded value.  I'm not sure what we're supposed to fix.

Comment 9

[(no longer active)]

I filed bug 569154 about escape encoding nbsp to 0xA0.

Comment 10

[Arpad Borsos [:Swatinem]]

Attachment: use encodeURIComponent

Comment 11

[Arpad Borsos [:Swatinem]]

Could you please test if this patch fixes the use of json_encode and reland the original patch?

Comment 12

[(no longer active)]

Comment on attachment 448352 [details] [diff] [review]
use encodeURIComponent

Yes, it seems to work fine.  Thanks!

Comment 13

[(no longer active)]

So, things seemed to work fine, and I relanded this with the new patch combined.

http://hg.mozilla.org/users/mstange_themasta.com/tinderboxpushlog/rev/58be23887d91

Comment 14

[Markus Stange [:mstange]]

deployed

Comment 15

[(no longer active)]

Oh, this broke the tbpl again.

{"error":1,"code":"Client","message":"Application failed during request deserialization: \nnot well-formed (invalid token) at line 3, column 64, byte 282 at /usr/lib64/perl5/vendor_perl/5.8.8/x86_64-linux-thread-multi/XML/Parser.pm line 187\n"}

http://hg.mozilla.org/users/mstange_themasta.com/tinderboxpushlog/rev/5ed9db12894b

Markus, could you please deploy this?

And Arpad, let's just not use native json functions here.  It's really not worth it.

Comment 16

[Markus Stange [:mstange]]

(In reply to comment #15)
> Markus, could you please deploy this?

Done.

Comment 17

[Ed Morley [:emorley]]

Going to reopen this so it appears on my searches & can be looked at in the future, on the off-chance newer php versions have fixed the issues with native json.

Comment 18

[Ed Morley [:emorley]]

Is there any reason why we can't |json_encode(utf8_encode($foo))|, per the last comment in https://bugs.php.net/bug.php?id=51947 ?

Comment 19

[(no longer active)]

(In reply to comment #18)
> Is there any reason why we can't |json_encode(utf8_encode($foo))|, per the last
> comment in https://bugs.php.net/bug.php?id=51947 ?

Hmm yeah that might work!

Comment 20

[Ed Morley [:emorley]]

Wontfix since TBPL is being replaced by Treeherder.