1360961 - Crash in js::GetPrototype during cross-compartment hasInstance

Status: RESOLVED FIXED

(Core :: JavaScript: GC, defect)

Description

[Marcia Knous [:marcia]]

This bug was filed from the Socorro interface and is 
report bp-6df16c20-2d29-4c39-bf18-6c9f70170430.
=============================================================

Seen while looking at nightly crash stats - crashes started using 20170427030231: http://bit.ly/2oW0ck8

Possible regression ranged based on Build ID: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=0f5ba06c4c5959030a05cb852656d854065e2226&tochange=0b77ed3f26c5335503bc16e85b8c067382e7bb1e

Comment 1

[Andrew McCreight [:mccr8]]

I see a number of crashes with this signature with GC poison values like 0x4b4b4b4f:
  bp-59d8d642-6f40-4128-9b89-dbcce0170501

All of the crashes I looked at were cross-compartment hasInstance calls.

Bug 1352430 is in the range. Maybe that's the issue?

Comment 2

[Jon Coppeard (:jonco)]

(In reply to Andrew McCreight [:mccr8] from comment #1)
Thanks, that is the likely culprit.

Comment 3

[Andrew McCreight [:mccr8]]

[Tracking Requested - why for this release]: This signature is older, but there's a new spike that looks like a sec-high.

Comment 4

[Marcia Knous [:marcia]]

Tracking 55+ based on Comment 3.

Comment 5

[Jon Coppeard (:jonco)]

Attachment: 1 - has-instance-assertions

Not a fix.  Add compartment asserts to JS::OrdinaryHasInstance (on the stack for  this crash) and make all compartment asserts check for callers passing in dying objects.

Comment 6

[Jon Coppeard (:jonco)]

Attachment: 2 - barrier-plugin-wrappers

I found that plugin object wrappers are also stored in a map and so that requires barriering in the same way as the wrapper cache: a check for dying objects in nsNPObjWrapper::GetNewOrUsed and conditional removal of entries in NPObjWrapper_Finalize if they still point to the dying object.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8864199 [details] [diff] [review]
2 - barrier-plugin-wrappers

>+      entry->mJSObj = nullptr;

I guess this is OK because we're guaranteed to either set mJSObj after this point or to remove "entry" before returning from this function?  Otherwise we'd end up never removing the entry in the finalizer...

May be worth documenting that.

r=me

Comment 8

[Jon Coppeard (:jonco)]

(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #7)
Good point, I'll add a comment.

Comment 9

[Jon Coppeard (:jonco)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bb4b18910a1bbab19d851a3a9740c4db61ad64e6
https://hg.mozilla.org/integration/mozilla-inbound/rev/2aac093eff951f0f1353059347c4984e520bab7e
https://hg.mozilla.org/integration/mozilla-inbound/rev/5f2914fb211dafdc43d26087e68f174384526156

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/bb4b18910a1b
https://hg.mozilla.org/mozilla-central/rev/2aac093eff95
https://hg.mozilla.org/mozilla-central/rev/5f2914fb211d

Comment 11

[Carsten Book [:Tomcat]]

urgh missed leave-open sorry

Comment 13

[User Dderss]

Can anyone please look at this crash report:

https://crash-stats.mozilla.com/report/index/c914b45a-ab86-4579-b399-c5f971170601

If this will be fixed in FF 55 as per this bug, can the fix be applied to FF 53, too, so I would not wait for three months before FF 55 will become a final release version?

Comment 14

[Andrew McCreight [:mccr8]]

(In reply to User Dderss from comment #13)
> If this will be fixed in FF 55 as per this bug, can the fix be applied to FF
> 53, too, so I would not wait for three months before FF 55 will become a
> final release version?

This bug was about a particular instance of this crash signature that only affected 55. If you are seeing a similar crash in an earlier version, you should file a separate bug. However, without more information it will likely be difficult for us to fix it.