Closed
Bug 702572
Opened 13 years ago
Closed 13 years ago
Minimized prototype.js broken in Firefox 9+
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
VERIFIED
FIXED
mozilla11
People
(Reporter: lh, Assigned: dmandelin)
References
Details
(Keywords: verified-aurora, verified-beta, Whiteboard: [qa!])
Attachments
(3 files, 2 obsolete files)
(deleted),
text/html
|
Details | |
(deleted),
text/html
|
Details | |
(deleted),
patch
|
jorendorff
:
review+
akeybl
:
approval-mozilla-aurora+
jst
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:9.0) Gecko/20100101 Firefox/9.0
Build ID: 20111109112850
Steps to reproduce:
Video of the bug : http://screencast.com/t/gsgffQZM
URL : http://www.ajaxplorer.info/demo/
Actual results:
This function is existing, and not existing :/
Expected results:
This page worked till the last Firefox 9 beta version of this morning.
Function ... worked. Now it says it's not existing, but it is.
Summary: Javascript bad behaviour → Functions can exists and not exists at the same time.
Summary: Functions can exists and not exists at the same time. → Functions can exist and not exist at the same time.
I unpacked the js and now it works...
Status: UNCONFIRMED → RESOLVED
Closed: 13 years ago
Resolution: --- → INVALID
Comment 2•13 years ago
|
||
I can confirm the "Form.serialize is not a function" message in Nightly builds, even if I add the JS to a simple HTML file. We should understand what's happening here before this reaches the stable channel.
Do you know what minimizer was used here?
Status: RESOLVED → REOPENED
Ever confirmed: true
Resolution: INVALID → ---
Comment 3•13 years ago
|
||
This regressed on the Aurora branch. Still bisecting...
Comment 4•13 years ago
|
||
11/01 WORKS - http://hg.mozilla.org/releases/mozilla-aurora/rev/c8fcdb6bd4d7
11/02 FAILS - http://hg.mozilla.org/releases/mozilla-aurora/rev/90a4c98c1ae3
http://hg.mozilla.org/releases/mozilla-aurora/pushloghtml?fromchange=c8fcdb6bd4d7&tochange=90a4c98c1ae3
Almost certainly bug 684489.
Blocks: 684489
Updated•13 years ago
|
status-firefox10:
--- → affected
status-firefox8:
--- → unaffected
status-firefox9:
--- → affected
tracking-firefox10:
--- → ?
tracking-firefox9:
--- → ?
Comment 5•13 years ago
|
||
Standalone HTML file. Shows "Form.serialize is not a function" in the web console in Firefox 9+.
Lucien, thanks for the bug report!
Comment 6•13 years ago
|
||
Anybody willing to look into this? It definitely seems to be a regression, and the combination minimizerX + prototype.js may be very common...
Updated•13 years ago
|
Summary: Functions can exist and not exist at the same time. → Minimized prototype.js broken in Firefox 9+
Assignee | ||
Updated•13 years ago
|
Updated•13 years ago
|
Assignee: general → jorendorff
Comment 7•13 years ago
|
||
Comment 8•13 years ago
|
||
Shell testcase:
eval("function f() { function g() {} return g; }");
assertEq(f().prototype !== f().prototype, true);
Comment 9•13 years ago
|
||
Related:
function f() {
function g() {
function h() { return h; }
}
return g;
}
assertEq(f().prototype !== f().prototype, true);
The first bad revision is:
changeset: 37685:36bbd730e24f
user: Brendan Eich <brendan@mozilla.org>
date: Thu Jan 14 09:33:14 2010 -0800
summary: Analyze module pattern and private-statics pattern in order to despecialize from methods to slots/sprops (536564, r=jorendorff).
Assignee | ||
Comment 10•13 years ago
|
||
This is a bad bug for the browser (as is the hopefully dup bug 706972), and I want to get it fixed for 9 if release-drivers will take it.
The attached strawman patch fixes the test case in comment 8 and passes shell tests. I think it might work, suitably cleaned up. Explanation of what I found:
In the pre-regression code, in SetFunctionKinds we go to the final case and g ends up getting optimized to a null closure. This is AIUI, wrong. But continuing from there, in JSOP_DEFLOCALFUN, being a null closure takes us into the first arm of the if, which always clones. The functions in the test case thus are unequal, and the assertion passes.
In the post-regression code, in SetFunctionKinds we take the 2nd case (with newly weakened condition) and don't optimize. Then in JSOP_DEFLOCALFUN, we take the second arm of the if, which optimizes by cloning only if the function parent is different. I believe that in this case, because we are using DEFLOCALFUN and g doesn't use any variables defined in f, f is not marked heavyweight, so it doesn't get a call object. Thus, both copies of g get parented to the call object of the eval (I think, didn't check that, but if not, it would be the global object). So the optimization applies, and we don't clone. The result is the functions come out identical, and the assertion fails.
I'm not sure where the bug actually is (and there are probably multiple answers to that question), but I figure that when we don't clone, a read barrier is supposed to fire at some point to clone if needed. I think those are only on properties, so they will not apply to the object created by DEFLOCALFUN, which is in a local variables. I would vaguely guess that the optimization in DEFLOCALFUN is just wrong, but was masked by the regressing bug.
Assignee | ||
Comment 11•13 years ago
|
||
Assignee: jorendorff → dmandelin
Attachment #578475 -
Attachment is obsolete: true
Status: REOPENED → ASSIGNED
Attachment #578732 -
Flags: review?(jorendorff)
Comment 12•13 years ago
|
||
We've gotten a couple of 100% CPU reports caused by this bug (706911 and 706972). We're more likely to take this in FF9 if reviewed and landable tomorrow before we go to build for the second to last FF9 beta.
Please make sure to include a risk assessment when nominating. Thanks!
Comment 13•13 years ago
|
||
Comment on attachment 578732 [details] [diff] [review]
Patch
Excellent. I love it. Methodjit needs to be updated too though.
Attachment #578732 -
Flags: review?(jorendorff)
Assignee | ||
Comment 14•13 years ago
|
||
Comment on attachment 578732 [details] [diff] [review]
Patch
Jason pointed out on IRC that we also need to update DefLocalFun in the JM stubs.
Assignee | ||
Comment 15•13 years ago
|
||
With methodjit change. Also, I realized that v1 had removed a null check on the output of GetScopeChainFast, so I fixed that.
Attachment #578732 -
Attachment is obsolete: true
Attachment #579391 -
Flags: review?(jorendorff)
Comment 16•13 years ago
|
||
Comment on attachment 579391 [details] [diff] [review]
Patch v2
>- obj = CloneFunctionObjectIfNotSingleton(f.cx, fun, &f.fp()->scopeChain());
>- if (!obj)
>- THROWV(NULL);
>+ parent = &f.regs.fp()->scopeChain();
> } else {
>- JSObject *parent = GetScopeChainFast(f.cx, f.fp(), JSOP_DEFLOCALFUN,
>- JSOP_DEFLOCALFUN_LENGTH);
>+ parent = GetScopeChainFast(f.cx, f.regs.fp(), JSOP_DEFLOCALFUN,
>+ JSOP_DEFLOCALFUN_LENGTH);
In both places f.regs.fp() could just be written f.fp().
r=me either way.
Attachment #579391 -
Flags: review?(jorendorff) → review+
Comment 17•13 years ago
|
||
Is this somewhat like bug 694454? We keep finding bugs with prototype & walking up the scope chain. Do we not have good test cases here? Are they related?
Assignee | ||
Comment 18•13 years ago
|
||
(In reply to Christopher Blizzard (:blizzard) from comment #17)
> Is this somewhat like bug 694454? We keep finding bugs with prototype &
> walking up the scope chain. Do we not have good test cases here? Are they
> related?
Luke has already started an overhaul of scope chains--things should be simpler and more reliable when he is done.
Assignee | ||
Comment 19•13 years ago
|
||
Comment 20•13 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 13 years ago → 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla11
Assignee | ||
Comment 21•13 years ago
|
||
Comment on attachment 579391 [details] [diff] [review]
Patch v2
Nominating for landing to Aurora and Beta. Benefits:
- Breaking minimized prototype.js could break a lot of sites, so fixing it is good.
- We already have another site that hangs (bug 706911) due to this bug and is fixed by this patch.
- We could consider backing out the regressing patch (the one that caused this bug), but that patch fixes a regression in Google Maps, so that's not very appealing.
Risks are low:
- A small risk is that some site out there depends on the old buggy behavior and will be broken by the new behavior.
- The patch is only about 20 lines of code, and it's mostly 2 parts duplicated, so really only 10 lines to think about.
- The effect of the patch is just to disable an optimization that rarely comes up in practice (so the expected impact on performance is negligible).
Attachment #579391 -
Flags: approval-mozilla-beta?
Attachment #579391 -
Flags: approval-mozilla-aurora?
Comment 22•13 years ago
|
||
Comment on attachment 579391 [details] [diff] [review]
Patch v2
[Triage Comment]
Approving for Aurora since we'd want to get testing there before considering for the next beta build at this point.
We'll start weighing the options for beta (taking this vs backing out) at tomorrow's channel meeting.
Attachment #579391 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Assignee | ||
Comment 24•13 years ago
|
||
status-firefox11:
--- → fixed
Comment 25•13 years ago
|
||
Comment on attachment 579391 [details] [diff] [review]
Patch v2
Approved for beta per todays drivers meeting. While this is not a trivial fix, it feels better to take this than to back out the patch that caused this.
Attachment #579391 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Assignee | ||
Comment 26•13 years ago
|
||
Comment 27•13 years ago
|
||
(In reply to David Mandelin from comment #26)
> http://hg.mozilla.org/releases/mozilla-beta/rev/77d42322277d
A note to QA testers, this fix will not be verifiable until we have 9.0b6 builds.
Comment 28•13 years ago
|
||
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:9.0) Gecko/20100101 Firefox/9.0
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:10.0a2) Gecko/20111214 Firefox/10.0a2
Verified the fix on the above builds and issue is no longer reproducible: web console des not return "Form.serialize() is not a function" anymore
Whiteboard: [qa+] → [qa]
Comment 29•13 years ago
|
||
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:9.0) Gecko/20100101 Firefox/9.0
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:10.0a2) Gecko/20111214 Firefox/10.0a2
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:11.0a1) Gecko/20111214 Firefox/11.0a1
Verified the fix on the above builds and issue is no longer reproducible: web console des not return "Form.serialize() is not a function" anymore
Reporter | ||
Comment 30•13 years ago
|
||
Thanks a lot ;)
You need to log in
before you can comment on or make changes to this bug.
Description
•