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

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

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

Comment 2

[Brian Hackett [Laid off!]]

Attachment: 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.

Comment 3

[Brian Hackett [Laid off!]]

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.

Comment 4

[Brian Birtles (:birtles, Feb 26~ parental leave)]

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.

Comment 5

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Sorry, cached form values somehow undid Boris' change to the tracking flags!

Comment 6

[Alex Keybl [:akeybl]]

What is the user impact of this bug?

Comment 7

[Luke Wagner [:luke]]

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?

Comment 8

[Brian Hackett [Laid off!]]

(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.

Comment 9

[Brian Birtles (:birtles, Feb 26~ parental leave)]

(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.

Comment 10

[Brian Hackett [Laid off!]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/dc2865fc9538

Comment 11

[Alex Keybl [:akeybl]]

(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?

Comment 12

[Alex Keybl [:akeybl]]

(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.

Comment 13

[Alex Keybl [:akeybl]]

Brian Hackett*

Comment 14

[Brian Hackett [Laid off!]]

This shouldn't be high risk, but it should bake in the nightlies for at least a few days.

Comment 15

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/dc2865fc9538

Comment 17

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Confirming that this bug fixes bug 707880 (SVG wallpaper maker broken). I hope this fix can be landed on beta.

Comment 18

[Alex Keybl [:akeybl]]

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.

Comment 19

[Alex Keybl [:akeybl]]

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.

Comment 20

[Brian Birtles (:birtles, Feb 26~ parental leave)]

(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.

Comment 21

[Christian Holler (:decoder)]

(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.

Comment 22

[Alex Keybl [:akeybl]]

(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.

Comment 23

[Brian Hackett [Laid off!]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/1b4a19a9295a

Comment 24

[LegNeato]

Do we need this for FIrefox 9 as well? The dupe chain goes back to Fx9. If so, we need to know ASAP.

Comment 25

[David Mandelin [:dmandelin]]

(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.

Comment 26

[Brian Birtles (:birtles, Feb 26~ parental leave)]

(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

Comment 27

[Brian Hackett [Laid off!]]

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; }
}

Comment 28

[Brian Birtles (:birtles, Feb 26~ parental leave)]

(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.

Comment 30

[Frank Wienberg]

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?).

Comment 31

[Virgil Dicu [:virgil] [QA]]

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.

Comment 32

[cdumoulin]

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

Comment 33

[Alex Keybl [:akeybl]]

(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.