1087310 - Assertion failure: obj->isNative(), at c:\mozilla\builds\nightly\mozilla\js\src\gc/Marking.cpp:974

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Carsten Book [:Tomcat]]

Attachment: bughunter stack

found via bughunter.

Steps to reproduce (Windows 7 Debug Build latest Trunk)
-> Load http://www.redplanet.gr/podosfairo/parwn-o-ampintal-me-gioyventoys-kai-panathhnaiko.3092445.html
-->>> Assertion failure: obj->isNative(), at c:\mozilla\builds\nightly\mozilla\js\src\gc/Marking.cpp:974

will test if i can get a testcase etc

Comment 1

[Carsten Book [:Tomcat]]

Attachment: windgb data win 7 trunk debug build

Comment 2

[Carsten Book [:Tomcat]]

marking sec-sensitive just in case. Seems older Assertion failure: obj->isNative() were sec bugs before 

Jandem, bhacket could you take a look at this. Thanks!

Comment 3

[Carsten Book [:Tomcat]]

Attachment: first reduced local testcase

Comment 4

[Brian Hackett [Laid off!]]

This is a GGC bug.  Here is what happens:

1) We create a tenured native object A (a DOM object) and stick a nursery pointer in one of its slots.
2) This causes a SlotsEdge entry to be added to the store buffer.
3) We then swap A with another object B, a proxy.
4) When we do a nursery collection, there is a SlotsEdge with a pointer to a proxy and a slot range that no longer makes any sense.

The assertion failure is new as of bug 1073842, but the problem goes back before that.  Since bug 1073842 uses a layout for proxies that mimics that of normal native objects, the same catastrophes can befall us now that befell us earlier.  If user scripts can access object A before it is swapped, this is a huge security hole (both now and before bug 1073842.)  If they can't, this is still a big problem that prevents using better proxy layouts, and won't be easy to fix.  A couple options are doing a nursery GC whenever we swap two objects, or removing the SlotsEdge store buffer entirely.  I would be in favor of the latter option as I don't think this store buffer does anything that the whole cell store buffer doesn't do better (the JIT doesn't even use the SlotsEdge store buffer).

JSObject::swap is just such an incredibly dangerous API, for this and other reasons, and it would be nice if we had more forcible constraints for reasoning about when it can be used.

Comment 5

[Terrence Cole [:terrence]]

Last time I tried removing the SlotsEdge buffer, I saw a massive regression on PDF.js because of all the slice operations it does. The SlotsEdges should be clipping themselves to the valid range of the current object so it's probably just a bug in that logic.

Comment 6

[Brian Hackett [Laid off!]]

Attachment: patch

Ah, I didn't notice that SlotsEdge was restricting the slots it marked to those currently valid in the object.

Comment 7

[Terrence Cole [:terrence]]

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

Review of attachment 8509774 [details] [diff] [review]:
-----------------------------------------------------------------

Perfect!

::: js/src/gc/StoreBuffer.cpp
@@ +27,5 @@
>      NativeObject *obj = object();
>  
> +    // Beware JSObject::swap exchanging a native object for a non-native one.
> +    if (!obj->isNative())
> +        return; 

Extra whitespace at eol.

Comment 8

[Brian Hackett [Laid off!]]

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

Comment 9

[Ryan VanderMeulen [:RyanVM]]

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

Seems like this needs a sec rating, sec-approval (depending on the rating), and branch nominations, yes?

Comment 10

[Brian Hackett [Laid off!]]

No, actually, since SlotsEdge does restrict the slots it marks to those which are present on the object, this is a bogus assert.  Non-native objects have a slot span of zero.