Minimized prototype.js broken in Firefox 9+

VERIFIED FIXED in Firefox 9

Status

()

Core
JavaScript Engine
VERIFIED FIXED
6 years ago
6 years ago

People

(Reporter: Lucien, Assigned: dmandelin)

Tracking

({verified-aurora, verified-beta})

9 Branch
mozilla11
x86_64
Windows 7
verified-aurora, verified-beta
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox8 unaffected, firefox9+ fixed, firefox10+ fixed, firefox11+ fixed)

Details

(Whiteboard: [qa!])

Attachments

(3 attachments, 2 obsolete attachments)

(Reporter)

Description

6 years ago
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.
(Reporter)

Updated

6 years ago
Summary: Javascript bad behaviour → Functions can exists and not exists at the same time.
(Reporter)

Updated

6 years ago
Summary: Functions can exists and not exists at the same time. → Functions can exist and not exist at the same time.
(Reporter)

Comment 1

6 years ago
I unpacked the js and now it works...
Status: UNCONFIRMED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → INVALID
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 → ---
This regressed on the Aurora branch. Still bisecting...
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

6 years ago
status-firefox10: --- → affected
status-firefox8: --- → unaffected
status-firefox9: --- → affected
tracking-firefox10: --- → ?
tracking-firefox9: --- → ?
Created attachment 574586 [details]
Testcase

Standalone HTML file. Shows "Form.serialize is not a function" in the web console in Firefox 9+.

Lucien, thanks for the bug report!
Anybody willing to look into this? It definitely seems to be a regression, and the combination minimizerX + prototype.js may be very common...

Updated

6 years ago
Summary: Functions can exist and not exist at the same time. → Minimized prototype.js broken in Firefox 9+
(Assignee)

Updated

6 years ago
tracking-firefox10: ? → +
tracking-firefox11: --- → +
tracking-firefox9: ? → +
Assignee: general → jorendorff
Created attachment 575477 [details]
unpacked testcase, still triggers the error
Shell testcase:

  eval("function f() { function g() {} return g; }");
  assertEq(f().prototype !== f().prototype, true);
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

6 years ago
Created attachment 578475 [details] [diff] [review]
Strawman patch

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

6 years ago
Created attachment 578732 [details] [diff] [review]
Patch
Assignee: jorendorff → dmandelin
Attachment #578475 - Attachment is obsolete: true
Status: REOPENED → ASSIGNED
Attachment #578732 - Flags: review?(jorendorff)

Updated

6 years ago
Blocks: 706911
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 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

6 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

6 years ago
Created attachment 579391 [details] [diff] [review]
Patch v2

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 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+
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

6 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

6 years ago
http://hg.mozilla.org/integration/mozilla-inbound/rev/bab6fef4017c
https://hg.mozilla.org/mozilla-central/rev/bab6fef4017c
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla11
Whiteboard: [qa+]
(Assignee)

Comment 21

6 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 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)

Updated

6 years ago
Duplicate of this bug: 706911
(Assignee)

Comment 24

6 years ago
http://hg.mozilla.org/releases/mozilla-aurora/rev/9d149d145f6c
status-firefox10: affected → fixed
status-firefox11: --- → fixed
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

6 years ago
http://hg.mozilla.org/releases/mozilla-beta/rev/77d42322277d
status-firefox9: affected → fixed
(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.
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]
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
Status: RESOLVED → VERIFIED
Keywords: verified-aurora, verified-beta
Whiteboard: [qa] → [qa!]
(Reporter)

Comment 30

6 years ago
Thanks a lot ;)
You need to log in before you can comment on or make changes to this bug.