898047 - undefined variable, when it should have an Object value

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Comment 1

[David Iglesias]

We can provide with custom builds of the code upon request (non-minimzed code, whatever), but those should not be disclosed (for obvious reasons :P)

Comment 2

[Benjamin Smedberg]

This looks like a race condition in your code. It is very unlikely to be related to OOM conditions. In any case, I'm going to hide comment 0 because of the credentials and make the rest of this bug public. I'm also going to mark the bug INCOMPLETE because there's not enough evidence of a client-side bug, and Mozilla engineering doesn't have the resources to debug the site for you.

I suggest that you should probably debug it locally including logging all asynchronous activity, and file a bug with a reduced testcase if you discover an actual client-side bug.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Creating reduced testcases for JIT bugs can be pretty hard.  And the fact that turning off the ion JIT makes the bug disappear is pretty indicative of this being a bug on our end.  Furthermore, the number of friends likely affects how many times some functions run which can affect whether they get ion-compiled or not.  So reopening for the moment; I think there is a very good chance this is a JIT bug.

David, can you reproduce the bug in an Aurora or Nightly build?  I assume the bug is not present in 22 but is present in a 23 beta?

It would be very helpful to use http://mozilla.github.io/mozregression/ to narrow down when the problem appeared, and if it's no longer present in Nightly when it disappeared.

My bets on the former are when we made Ion compilation more likely by turning on the baseline compiler, for what that's worth.  :(

Comment 4

[Boris Zbarsky [:bzbarsky]]

For some reason Bugzilla isn't letting David comment, but here's what he has mailed me so far:

  FF 22 works just fine, I tested with all 23 betas (1 to 8) and they all fail. I tested
  current aurora (24 latest) and it fails too.

  This code worked just fine on Aurora 23 a 2 (because I worked a lot with aurora a while
  back to integrate WebRTC on the Chat, and we needed FF23 for TURN support), and just 
  confirmed it today.

which makes it less likely that the BC landing caused this, if it first appeared in 23 after the Aurora branch...  On the other hand, this means it was caused by something we backported.

I did just check that I see the problem in a 7/15 nightly (which is the most recent I have on hand right this second).

Comment 5

[David Iglesias]

OK I've finally completed the mozregression:

Last good nightly: 2013-04-24
First bad nightly: 2013-04-25

Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=fef5f202b2dc&tochange=690b5e0f6562

My environment is not ready to build nightlies on a per-commit basis (that's why I can only give you the nightly that broke stuff) but I could try to set it up, if you point me to some documentation that I can follow :) I'm on MacOSX (and the regression was done on my machine).

If needed I can run another regression test on a Linux box, and if really *really* needed I could try this too on a windows VM :)

Comment 6

[Till Schneidereit [:till]]

Based on that regression range, I'm tagging jonco. Since I can't see the page this is occurring on, I can't be sure, but rev c307cb8bffec looks like it could well be the culprit.

@jonco, if you don't think this is you, you could tag jandem for rev 4bba65656e46. ;)

Comment 7

[Jan de Mooij [:jandem]]

(In reply to Till Schneidereit [:till] from comment #6)
> @jonco, if you don't think this is you, you could tag jandem for rev
> 4bba65656e46. ;)

Bug 860145 is also in that range, jonco landed it but didn't write the patch. needinfo?-ing myself to bisect this later today.

Comment 8

[Jan de Mooij [:jandem]]

I can reproduce with a nightly build. Bisecting the range in comment 5 now.

Comment 9

[Lukas Blakk [:lsblakk] use ?needinfo]

Tracking - let's find out what we uplifted that needs to be backed out before our final beta (and ultimately RC) that goes to build on Monday.

Comment 10

[Jan de Mooij [:jandem]]

Bisection points to bug 858551. Bug 858551 made our Ion <-> Baseline calls sane, the JITs now use exactly the same code to call into Baseline or Ion.

This allowed us to remove some hacks from the JM+Ion days: JM <-> Ion calls were bad for performance so we had some workarounds in place to either invalidate the caller or increment the use count of the callee faster. See the changes to VMFunctions.cpp for the details.

The hack to invalidate the Ion code of the caller was only enabled if the JM pref was turned on. With Baseline enabled, JM has been effectively disabled (it aborts compilation) but the pref was still there and enabled by default.

So anyway, if I download a nightly from 2013-04-21, and I disable the javascript.options.methodjit.content pref, the site also stops working... So bug 858551 is a red herring and this is likely an older Ion bug. We need to bisect with JM preffed off. It's really late here though, I can try that next week or maybe tomorrow.

Comment 11

[Jan de Mooij [:jandem]]

(In reply to Jan de Mooij [:jandem] from comment #10)
> So bug 858551 is a red herring and this is likely an older Ion bug. We need to
> bisect with JM preffed off. It's really late here though, I can try that
> next week or maybe tomorrow.

Anybody should feel free to beat me to it btw.. We need to know what's going on here ASAP so any help here would be welcome.

Comment 12

[Jan de Mooij [:jandem]]

Attachment: Patch

Bug is that we optimized fun.apply(x, arguments) and did not set the Folded flag on the arguments MIR. Without the flag, we then eliminated the arguments phi and it became |undefined| after a bailout.

Wasn't easy to track down. I had to use a node.js HTTP proxy to add some logging calls to the website's JS code to track this down...

Comment 13

[Jan de Mooij [:jandem]]

Comment on attachment 782202 [details] [diff] [review]
Patch

[Approval Request Comment]
> Bug caused by (feature/regressing bug #):
Unknown..

> User impact if declined:
Websites not working. In this case the problem was some JS that's part of the YUI library so this may affect other websites that use YUI (or don't use YUI but use similar JS).

Testing completed (on m-c, etc.): 
> Didn't land yet.

Risk to taking this patch (and alternatives if risky):
> Very low.

String or IDL/UUID changes made by this patch:
> None.

Comment 14

[David Iglesias]

(In reply to Jan de Mooij [:jandem] from comment #12)
> Wasn't easy to track down. I had to use a node.js HTTP proxy to add some
> logging calls to the website's JS code to track this down...

Hey Jan, is there anything we can do on our side to make FF debugging easier?

Let me know (and if it's too sensitive feel free to contact me via email) so our code is friendlier the next time :)

Regards (and thanks everyone for the super-quick fix)!

Comment 15

[Jan de Mooij [:jandem]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9666b9de0feb

Comment 16

[Jan de Mooij [:jandem]]

(In reply to David Iglesias from comment #14)
> Hey Jan, is there anything we can do on our side to make FF debugging easier?
> 
> Let me know (and if it's too sensitive feel free to contact me via email) so
> our code is friendlier the next time :)

It's not really specific to your code. It's just that debugging JIT issues is easier if you have access to the JS source code. Also minimized code is harder to debug, but it's doable with something like jsbeautifier.org. Of course there are very good reasons for minimizing JS on production sites :)

Also, I know you offered access to non-minimized code in comment 1 but I wanted to try this first :)

> Regards (and thanks everyone for the super-quick fix)!

No problem, thank you for reporting this!

Comment 17

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/9666b9de0feb

Comment 18

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 782202 [details] [diff] [review]
Patch

let's get this uplifted asap for final beta build.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/604002b60b19
https://hg.mozilla.org/releases/mozilla-beta/rev/f7cb8dfa85dd

Comment 20

[David Iglesias]

Hey all, we've verified that our code works again just fine. Thanks again for the prompt resolution!

Any chance that somebody can redact user credentials from the original bug report? I'm not able to modify the description of the bug.

Thank you very much!

Comment 21

[Andrew McCreight [:mccr8]]

The first comment was marked private by bsmedberg, so only people in the security group will be able to view it.

Comment 22

[Boris Zbarsky [:bzbarsky]]

David, bug comments and descriptions are generally immutable (via the bugzilla UI; manual SQL queries on the database can change them, of course, but are generally not a really good idea).  As Andrew said, the set of people able to view the credentials in comment 0 is very restricted.  Gerv, is there more we can do here?

Comment 23

[Gervase Markham [:gerv]]

There is more we can technically do, but it's a hassle, it's a blunt instrument (nuking the entire comment) and our security group (the only people who can now see it) are a pretty trustworthy bunch :-)

Gerv

Comment 24

[David Iglesias]

That's great, nuking the entire comment is too extreme :)

Thanks again for all your help, regards!

Comment 25

[u279076]

David, can you confirm which versions of Firefox you verified? This should be fixed in Firefox 23, 24, and 25.

Comment 26

[u279076]

(In reply to Anthony Hughes, QA Mentor (:ashughes) from comment #25)
> David, can you confirm which versions of Firefox you verified? This should
> be fixed in Firefox 23, 24, and 25.

Canceling this long overdue request.