Closed Bug 898047 Opened 9 years ago Closed 9 years ago

undefined variable, when it should have an Object value

Categories

(Core :: JavaScript Engine, defect)

23 Branch
x86
All
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla25
Tracking Status
firefox23 + fixed
firefox24 + fixed
firefox25 + fixed

People

(Reporter: diglesias, Assigned: jandem)

Details

Attachments

(1 file)

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)
OS: Mac OS X → All
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.
Group: core-security
Status: UNCONFIRMED → RESOLVED
Closed: 9 years ago
Resolution: --- → INCOMPLETE
Restrict Comments: true
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.  :(
Status: RESOLVED → REOPENED
Ever confirmed: true
Flags: needinfo?(diglesias)
Resolution: INCOMPLETE → ---
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).
Restrict Comments: false
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 :)
Flags: needinfo?(diglesias)
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. ;)
Status: REOPENED → NEW
Flags: needinfo?(jcoppeard)
(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.
Flags: needinfo?(jdemooij)
I can reproduce with a nightly build. Bisecting the range in comment 5 now.
Flags: needinfo?(jdemooij)
Assignee: general → jdemooij
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.
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.
Flags: needinfo?(jcoppeard)
(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.
Attached patch PatchSplinter Review
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...
Attachment #782202 - Flags: review?(bhackett1024)
Status: NEW → ASSIGNED
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.
Attachment #782202 - Flags: approval-mozilla-beta?
Attachment #782202 - Flags: approval-mozilla-aurora?
Attachment #782202 - Flags: review?(bhackett1024) → review+
(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)!
(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!
https://hg.mozilla.org/mozilla-central/rev/9666b9de0feb
Status: ASSIGNED → RESOLVED
Closed: 9 years ago9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
Comment on attachment 782202 [details] [diff] [review]
Patch

let's get this uplifted asap for final beta build.
Attachment #782202 - Flags: approval-mozilla-beta?
Attachment #782202 - Flags: approval-mozilla-beta+
Attachment #782202 - Flags: approval-mozilla-aurora?
Attachment #782202 - Flags: approval-mozilla-aurora+
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!
The first comment was marked private by bsmedberg, so only people in the security group will be able to view it.
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?
Flags: needinfo?(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
Flags: needinfo?(gerv)
That's great, nuking the entire comment is too extreme :)

Thanks again for all your help, regards!
David, can you confirm which versions of Firefox you verified? This should be fixed in Firefox 23, 24, and 25.
Flags: needinfo?(diglesias)
(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.
Flags: needinfo?(diglesias)
You need to log in before you can comment on or make changes to this bug.