The default bug view has changed. See this FAQ.

JS Correctness: Different object types with/without TI and defineProperty/prototype

VERIFIED FIXED in Firefox 10

Status

()

Core
JavaScript Engine
--
critical
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: decoder, Assigned: bhackett)

Tracking

(Blocks: 1 bug, {testcase, verified-aurora, verified-beta})

Trunk
mozilla10
x86_64
Linux
testcase, verified-aurora, verified-beta
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox9+ affected, firefox10 verified, firefox11 verified)

Details

(Whiteboard: js-triage-needed [qa!])

Attachments

(1 attachment)

(Reporter)

Description

5 years ago
The following test produces different output with options "-m -a" vs. "-m -a -n" on mozilla-central revision ca140190529a:


function TestCase(a) {
    print(typeof(a));
}
Object.defineProperty(Object.prototype, "a", {});
new TestCase(eval("function t2(a,b) { this.a = b; }; x = new t2(1,3); x.a"));


Output:
$ $JS -m -a min.js
undefined
$ $JS -m -a -n min.js
number

Updated

5 years ago
Depends on: 706795
Duplicate of this bug: 706795
Created attachment 579133 [details] [diff] [review]
patch

Along with the issues described in bug 706795, definite properties were being added when the prototypes have a corresponding permanent property, where assignments to the property in the instance are inhibited by the permanent property.
Assignee: general → bhackett1024
Comment on attachment 579133 [details] [diff] [review]
patch

Let me know if I should punt this review to someone else.  It's a straight up TI bug but covers how TI state models shape/object semantics.
Attachment #579133 - Flags: review?(luke)
Blocks: 707880
tracking-firefox9: --- → ?
I've filed bug 707880 about Mozilla Japan's Foxkeh SVG Wallpaper Maker no longer working in Firefox 9 and, thanks to some help in identifying the cause from Brian Hackett, it seems like the root cause may be this bug.

Obviously we'd really like to see the fix shipped in Firefox 9.

I've filed a few further details and a further test case in bug 707880.
tracking-firefox9: ? → ---
Sorry, cached form values somehow undid Boris' change to the tracking flags!
tracking-firefox9: --- → ?

Comment 6

5 years ago
What is the user impact of this bug?

Comment 7

5 years ago
Comment on attachment 579133 [details] [diff] [review]
patch

>+            JSObject *parent = type->proto;
>+            while (parent) {
[snip]
>+                parent = parent->getProto();
>+            }

To test this, could you add extend the two tests you added to also test when the non-writable/setter-having property is 2 or more hops up the proto chain?
Attachment #579133 - Flags: review?(luke) → review+
(In reply to Luke Wagner [:luke] from comment #7)
> Comment on attachment 579133 [details] [diff] [review] [diff] [details] [review]
> patch
> 
> >+            JSObject *parent = type->proto;
> >+            while (parent) {
> [snip]
> >+                parent = parent->getProto();
> >+            }
> 
> To test this, could you add extend the two tests you added to also test when
> the non-writable/setter-having property is 2 or more hops up the proto chain?

This is already the case for the tests the fuzzer came up with.
(In reply to Alex Keybl [:akeybl] from comment #6)
> What is the user impact of this bug?

I guess websites breaking in surprising ways is one impact.

As per comment 4, at least the SVG wallpaper maker site is broken along with the SVG sprite component library. I imagine there may be many other such sites.

See the test case attached to bug 707880 (attachment 579221 [details]) for one example of what I imagine is fairly common Javascript pattern that is broken by this bug.

Furthermore, in the case of the SVG wallpaper maker site, the cause of the breakage was not obvious and took me several hours to trace it to this bug. So I suppose developer frustration is also a possible impact.
https://hg.mozilla.org/integration/mozilla-inbound/rev/dc2865fc9538
(In reply to Brian Birtles (:birtles) from comment #9)
> (In reply to Alex Keybl [:akeybl] from comment #6)
> > What is the user impact of this bug?
> 
> I guess websites breaking in surprising ways is one impact.
> 
> As per comment 4, at least the SVG wallpaper maker site is broken along with
> the SVG sprite component library. I imagine there may be many other such
> sites.
> 
> See the test case attached to bug 707880 (attachment 579221 [details]) for
> one example of what I imagine is fairly common Javascript pattern that is
> broken by this bug.
> 
> Furthermore, in the case of the SVG wallpaper maker site, the cause of the
> breakage was not obvious and took me several hours to trace it to this bug.
> So I suppose developer frustration is also a possible impact.

Thanks Brian. If we think that this bug has the possibility of affecting our user significantly, we should come up with a patch and consider taking for FF9/10. Would you mind addressing the risk of doing so?
tracking-firefox9: ? → +
(In reply to Alex Keybl [:akeybl] from comment #11)
> Would you mind addressing the risk of doing so?

This question should have of course been directed at Brian.
Brian Hackett*
This shouldn't be high risk, but it should bake in the nightlies for at least a few days.
https://hg.mozilla.org/mozilla-central/rev/dc2865fc9538
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla11
Attachment #579133 - Flags: approval-mozilla-beta?
Attachment #579133 - Flags: approval-mozilla-aurora?
Duplicate of this bug: 707880
Confirming that this bug fixes bug 707880 (SVG wallpaper maker broken). I hope this fix can be landed on beta.
Comment on attachment 579133 [details] [diff] [review]
patch

[Triage Comment]
Please land on aurora - we'll consider this for beta this afternoon PT and comment here at that time. Code freeze for beta is planned for Friday 12/9.
Attachment #579133 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 579133 [details] [diff] [review]
patch

[Triage Comment]
We don't have unknown web rendering issues/nominations on the tracking list right now so we're going to be risk averse and let this ride FF10. If there was widespread breakage, we think there would have been more bugs about rendering breakage.
Attachment #579133 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
(In reply to Alex Keybl [:akeybl] from comment #19)
> [Triage Comment]
> We don't have unknown web rendering issues/nominations on the tracking list
> right now so we're going to be risk averse and let this ride FF10. If there
> was widespread breakage, we think there would have been more bugs about
> rendering breakage.

Ok, understood. Is there a workaround available for this? If so that would be helpful to attach to the bug. (Since it won't be too impressive if Firefox can't render Mozilla's own website (bug 707880)!)

Personally, I'm somewhat surprised this hasn't caused problems with other websites.
(Reporter)

Comment 21

5 years ago
(In reply to Alex Keybl [:akeybl] from comment #19)
> We don't have unknown web rendering issues/nominations on the tracking list
> right now so we're going to be risk averse and let this ride FF10. If there
> was widespread breakage, we think there would have been more bugs about
> rendering breakage.


Strange, my intuition would have been that this causes problems on all sites that use defineProperty/prototype functionality. And as Brian Birtles pointed out even one of our own sites is affected by this bug.
(In reply to Christian Holler (:decoder) from comment #21)
> Strange, my intuition would have been that this causes problems on all sites
> that use defineProperty/prototype functionality. And as Brian Birtles
> pointed out even one of our own sites is affected by this bug.

It's too late in the cycle to take new risk to fix an issue affecting only one (known) web page. FF9 has been in the hands of users/testers/engineers for nearly 18 weeks now, so we decided to trust their (lack of) feedback on related web rendering issues.

We plan to go to build on Monday - if we run into other affected sites we can still consider taking this patch before then. Otherwise, this will be first fixed in FF10.
https://hg.mozilla.org/releases/mozilla-aurora/rev/1b4a19a9295a
Whiteboard: js-triage-needed → js-triage-needed [qa+]
Target Milestone: mozilla11 → mozilla10
status-firefox10: --- → fixed

Comment 24

5 years ago
Do we need this for FIrefox 9 as well? The dupe chain goes back to Fx9. If so, we need to know ASAP.
(In reply to Christian Legnitto [:LegNeato] from comment #24)
> Do we need this for FIrefox 9 as well? The dupe chain goes back to Fx9. If
> so, we need to know ASAP.

Comment 22 declared a decision not to fix this in Fx9, so action was planned.
(In reply to Brian Birtles (:birtles) from comment #20)
> Is there a workaround available for this? If so that would
> be helpful to attach to the bug.

Still seeking a workaround. We need to patch this site before Firefox 9 ships and it's getting late in the game.

Replacing each use of __defineSetter__ and the like with a regular function and likewise updating the call sites is not practical given the complexity of the site.[1]

If my hunch that other sites will be affected by this bug proves accurate---and provided those developers succeed in tracking down the problems to this bug---then such a workaround posted here would be useful to others too.

[1] The site in question is Mozilla Japan's: http://wallpapers.foxkeh.com/en/creator/pc.xhtml?w=1920&h=1080
The affected JS files are at least the following
http://wallpapers.foxkeh.com/assets/js/SVGSprite.js
http://wallpapers.foxkeh.com/assets/js/foxkehMaker/FoxkehMaker.js
http://wallpapers.foxkeh.com/assets/js/foxkehMaker/FoxkehMakerCalenderParts.js
If you wrap the constructor body in an unconditional 'if' statement, the optimization should be disabled and the setter should correctly be called.  The condition should not be 'true', as that will still get folded, but something like this should work:

function f() {
  if (0===0) { this.x = 0; this.y = 0; }
}
(In reply to Brian Hackett (:bhackett) from comment #27)
> If you wrap the constructor body in an unconditional 'if' statement, the
> optimization should be disabled and the setter should correctly be called. 
> The condition should not be 'true', as that will still get folded, but
> something like this should work:
> 
> function f() {
>   if (0===0) { this.x = 0; this.y = 0; }
> }

Thanks very much Brian, that's exactly what I was looking for.

Updated

5 years ago
Duplicate of this bug: 712929

Comment 30

5 years ago
Please note the discussion of bug 712929: there are people out there who are affected by this bug in Firefox 9, so please reconsider fixing this with a possible Firefox 9 update (9.0.2?).
Mozilla/5.0 (X11; Linux x86_64; rv:11.0a2) Gecko/20111228 Firefox/11.0a2
Mozilla/5.0 (X11; Linux x86_64; rv:10.0) Gecko/20100101 Firefox/10.0 Beta 1

verified using test case attachment from bug 707880:
https://bugzilla.mozilla.org/attachment.cgi?id=579221

The setter is now called in Firefox 10Beta 1 and Firefox Aurora.
Status: RESOLVED → VERIFIED
Keywords: verified-aurora, verified-beta
Whiteboard: js-triage-needed [qa+] → js-triage-needed [qa!] [qa!:10] [qa!:11]
status-firefox10: fixed → verified
status-firefox11: --- → verified
Whiteboard: js-triage-needed [qa!] [qa!:10] [qa!:11] → js-triage-needed [qa!]
status-firefox9: --- → affected

Comment 32

5 years ago
Just bouncing on how web pages are affected, I let you know that any project using google o3d WebGL implementation (see http://code.google.com/p/o3d/) is affected. I don't know if it is enough for you to reconsider fixing in FF 9 (hopefully yes because I'm stuck with my own project) but since o3d is the most popular starting point for WebGL implementations, it is something bad for most of people using FF 9 and WebGL. At least, you can test the fix with o3d samples...
(In reply to cdumoulin from comment #32)
> Just bouncing on how web pages are affected, I let you know that any project
> using google o3d WebGL implementation (see http://code.google.com/p/o3d/) is
> affected. I don't know if it is enough for you to reconsider fixing in FF 9
> (hopefully yes because I'm stuck with my own project) but since o3d is the
> most popular starting point for WebGL implementations, it is something bad
> for most of people using FF 9 and WebGL. At least, you can test the fix with
> o3d samples...

Only <2 weeks till this is fixed/released in FF10. We're not planning on releasing a 9.0.2 for this issue.
You need to log in before you can comment on or make changes to this bug.