727330 - "Assertion failure: (*dictp)->inDictionary()" when proxy fix returns a large array

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jesse Ruderman]

var a = [];
for (var i = 0; i < 200; ++i) a.push({});
var p = Proxy.create({fix: function() { return a; }});
Object.preventExtensions(p);

Assertion failure: (*dictp)->inDictionary(), at js/src/jsscopeinlines.h:359

The first bad revision is:
changeset:   ff51ddfdf5d1
user:        Brian Hackett
date:        Wed Sep 28 15:04:55 2011 -0700
summary:     Remove shape numbers and Shape::slotSpan, factor Shape getter/setter into BaseShape, bug 684505.

Comment 1

[Brian Hackett [Laid off!]]

Attachment: patch

Object swapping (as is done during proxy fixing) did not preserve the doubly linked structure of shape lists for objects in dictionary mode --- after swapping a link in the last property of one object could point to an internal field of the other object.  This issue predates bug 684505, since bug 684505 we generate a new Shape* when doing generateOwnShape, and failed when inserting that new shape into the broken dictionary.

Comment 2

[Brian Hackett [Laid off!]]

Comment on attachment 597997 [details] [diff] [review]
patch

[Approval Request Comment]
User impact if declined: Possible crashes, potential security exploit (after a GC pointers to freed memory could be used).
Risk to taking this patch (and alternatives if risky): Low, fixes incorrect state in an obscure corner of JS.

Comment 3

[Brian Hackett [Laid off!]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/75eb6956410b

Comment 4

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/75eb6956410b

Comment 5

[Alex Keybl [:akeybl]]

Adding [sg:] to find out the security rating of this bug before deciding on whether or not to uplift to Aurora. Since this is an internally reported bug and we're already on our fourth beta, denying for mozilla-beta for the sake of risk mitigation.

Comment 6

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(In reply to Brian Hackett (:bhackett) from comment #2)
> [Approval Request Comment]
> User impact if declined: Possible crashes, potential security exploit (after
> a GC pointers to freed memory could be used).

Brian mentions potential security exploit -> assuming worse case [sg:critical]

Comment 7

[Al Billings [:abillings - ex-MoCo]]

Alex, talking to Dan, we should probably take this for beta.

Comment 8

[Alex Keybl [:akeybl]]

Comment on attachment 597997 [details] [diff] [review]
patch

[Triage Comment]
The security team now recommends that we uplift as soon as possible, so we'll take this for both Aurora 12 and Beta 11 based upon the low risk assessment.

Comment 9

[Brian Hackett [Laid off!]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/76dddb5a8ae7
https://hg.mozilla.org/releases/mozilla-beta/rev/935e8d973a75

Comment 10

[Daniel Veditz [:dveditz]]

Need this fix on ESR as well.

Comment 11

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 597997 [details] [diff] [review]
patch

Please land this today in time for tomorrow's go-to-build.  See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for details and ping me or email [e-mail] if there are any concerns about getting this landed in time.

Comment 12

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Patch was backported to esr10, rebasing modifications were r=bhackett via IRC:

http://hg.mozilla.org/releases/mozilla-esr10/rev/c603b9cdee7f

Comment 13

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

> Patch was backported to esr10, rebasing modifications were r=bhackett via
> IRC:

Quickly landed to meet the March 2 build deadline, bhackett wasn't at a machine where he could do this today.

Comment 14

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(In reply to Gary Kwong [:gkw, :nth10sd] from comment #13)
> > Patch was backported to esr10, rebasing modifications were r=bhackett via
> > IRC:
> 
> Quickly landed to meet the March 2 build deadline, bhackett wasn't at a
> machine where he could do this today.

This was backed out for causing extreme fire.

https://hg.mozilla.org/releases/mozilla-esr10/rev/314eac275aab

Comment 15

[Brian Hackett [Laid off!]]

Oh, there's a bogus assert in esr10 when calling inDictionaryMode() on non-native objects.  Testing a->isNative() && a->inDictionaryMode() instead should work (and likewise for b).

Comment 16

[Lukas Blakk [:lsblakk] use ?needinfo]

Can someone give this another go?  Go to build is imminent and blocked on this landing.

Comment 17

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

> Can someone give this another go?  Go to build is imminent and blocked on
> this landing.

Landed:
http://hg.mozilla.org/releases/mozilla-esr10/rev/6dd1c5192f09
Backed out due to wrong fix:
http://hg.mozilla.org/releases/mozilla-esr10/rev/32cd852c1e6b

Gave it another go based on comment 15, hopefully it sticks on tbpl ( https://tbpl.mozilla.org/?tree=Mozilla-Esr10 ) this time:

http://hg.mozilla.org/releases/mozilla-esr10/rev/cdc9bf49ac24

Comment 18

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

> Gave it another go based on comment 15, hopefully it sticks on tbpl (
> https://tbpl.mozilla.org/?tree=Mozilla-Esr10 ) this time:
> 
> http://hg.mozilla.org/releases/mozilla-esr10/rev/cdc9bf49ac24

Looks green on tbpl, this stuck! :)

Comment 19

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Note to QA: Apparently this will be a little tricky to test before & after landing of the patch on ESR - on ESR 10, the assert doesn't seem to show both before and after the patch.

The testcase doesn't fail before the patch landed because objshrink (which I think landed after Firefox 10) is needed to trigger the issue, but the problem still exists on Fx 10, just that the symptom is not the assert.

Comment 20

[u279076]

Given comment 19, the short timeframe to release, and the fact that we are focusing on multiple releases here, we will skip QA verification for esr10; instead we will verify for Firefox 11 and 12, and assume verified for Firefox 10.0.3esr.

If someone has ideas how we can verify this on Firefox 10.0.3esr, I'm open to suggestions.

Comment 21

[u279076]

Verified fixed in Firefox 11.0b6, latest-12.0a2, and latest-13.0a1.

Comment 22

[Christian Holler (:decoder)]

Automatically extracted testcase for this bug was committed:

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