811050 - Assertion failure: thing->compartment()->rt == trc->runtime, at Marking.cpp:91

Status: RESOLVED FIXED

(Core :: DOM: Workers, defect)

Description

[Eitan Isaacson [:eeejay]]

I manage to bring up this failure with a tweaked mochitest.

Comment 1

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Please post stacks as an attachment, not as inline comments, and definitely not in the crash signature field.

Why do you think this is related to workers?  The crash is on the main thread.

Comment 2

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

I suppose it does involve different runtimes.  That smells workerish.

Comment 3

[Eitan Isaacson [:eeejay]]

Attachment: make test crash

I suspect GC needs to be forced in the worker, and I don't know how to do that. On my machine the assertion will come up after around 2000 test1 calls.

Comment 4

[Eitan Isaacson [:eeejay]]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #2)
> I suppose it does involve different runtimes.  That smells workerish.

Yeah, has something to do with the ByteArray having a view and neutering.

Comment 5

[Eitan Isaacson [:eeejay]]

Attachment: Back trace

Comment 6

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Steve, could you take a look at this? It seems like a worker object might be leaking into the main thread.

Comment 7

[Steve Fink [:sfink] [:s:]]

Eitan: thanks for the really nice test case!

The problem is that when transferring the contents of an ArrayBuffer to a new owner ArrayBuffer, it was inheriting the old buffer's list of views as well. I marked this s-s because it means you have an indirect way of accessing somebody else's memory (a view in the origin compartment until it gets GC'd, then some random memory after that.) It's probably pretty hard to do much with it, but it still smells a little dangerous.

The quick fix of NULLing out the view list when accepting a contents buffer seems to be running fine without crashing, but really that pointer should never be inside of a returned contents buffer in the first place.

Comment 8

[Steve Fink [:sfink] [:s:]]

Attachment: Initialize stolen ArrayBuffer data

Zero out the view list pointer before returning contents for JS_StealArrayBufferData

Comment 9

[Andrew McCreight [:mccr8]]

That sounds bad, so marking sec-high. Feel free to lower or raise as appropriate.

Comment 10

[Terrence Cole [:terrence]]

Steve, what branches are affected here and should we request tracking?

Comment 11

[Steve Fink [:sfink] [:s:]]

It goes back to beta, unfortunately.

It probably doesn't need to be sec-high, but I'm no expert. It only provides very indirect access to the data -- the only thing that value is used for is walking down the list of views to neuter them. So if the origin views are still alive, this will have no effect because the only thing you could do to them is neuter them, and they'll already be neutered. If they became garbage, then you could use this to write a couple of zeroes into the headers of new objects in the origin compartment. Bad, but I lack the imagination to see how that's exploitable.

Comment 12

[Ben Turner (not reading bugmail, use the needinfo flag!)]

I added additional rooting to workers in bug 814026 and had a bizarre round of additional crashes that caused it to be backed out... Is there some chance that this is the underlying cause?

Comment 13

[Steve Fink [:sfink] [:s:]]

Seems unlikely. The immediate memory corruption that this would cause would be in objects of the same alloc kind, and you're seeing crashes in JSStrings. (You *are* using workers, though, which is the only thing I know of that steals buffers right now. So it's not out of the question that this is affecting you.)

Comment 14

[Steve Fink [:sfink] [:s:]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/b3d02828b6f3

Comment 15

[Ed Morley [:emorley]]

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

Comment 16

[Steve Fink [:sfink] [:s:]]

Comment on attachment 681267 [details] [diff] [review]
Initialize stolen ArrayBuffer data

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 720949
User impact if declined: crashes, security hole
Testing completed (on m-c, etc.): on m-c, test case passes
Risk to taking this patch (and alternatives if risky): low. This is NULLing out an invalid pointer, preventing traversal through it to dangerous lands.
String or UUID changes made by this patch: none

Comment 17

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 681267 [details] [diff] [review]
Initialize stolen ArrayBuffer data

low risk - go ahead with landing today so it gets into beta 4 tomorrow.

Comment 18

[Steve Fink [:sfink] [:s:]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/5a3041179998

Comment 19

[Steve Fink [:sfink] [:s:]]

http://hg.mozilla.org/releases/mozilla-beta/rev/2e8dc13c7b5c