638324 - JS_Enumerate doesn't root the JSIdArray it returns

Status: RESOLVED WORKSFORME

(Core :: JavaScript Engine, defect)

Description

[Andreas Gal :gal]

This is a great foot gun since we don't offer any JS API-way of rooting JSIdArrays, people tend to write code that ends up not rooting the array:

http://mxr.mozilla.org/mozilla-central/source/modules/plugin/base/src/nsJSNPRuntime.cpp#964

Here we get an id array and then call into JS, which might GC.

http://mxr.mozilla.org/mozilla-central/source/js/src/xpconnect/src/xpcwrappedjsclass.cpp#434

Dito here.

http://mxr.mozilla.org/mozilla-central/source/xpinstall/src/nsJSInstallTriggerGlobal.cpp#291

Same here.

Basically every use of JS_Enumerate outside the engine is unsafe.

There is a couple ways we can fix this, including handing back an object (thats what JS_NewPropertyEnumerator tries to do, but that API is really ugly too).

I think a nice API is for JS_Enumerate to automatically root the IdArray in the context and JS_DestroyIdArray will unroot it.

Comment 1

[Andreas Gal :gal]

Also, I have a hunch that this might be responsible for our #10 top crasher, bug 615098, which is all about MarkId failing. These id arrays contain ... well ids. I don't want to link the dependency in the other bug since this is sg:crit.

Comment 2

[Andreas Gal :gal]

Another unrooted use:

http://mxr.mozilla.org/mozilla-central/source/content/html/content/src/nsHTMLCanvasElement.cpp#495

Comment 3

[Andreas Gal :gal]

I conferred with mrbkap. We think that this is not critical. ids are strings, minus the * object, so its not possible to do any code injection with this. But I still think this might be the top crasher. Asking for 2.0.x and 1.9.1 1.9.2 blocking.

Comment 4

[Andreas Gal :gal]

Attachment: patch

This patch rips out an API that is unused and that is totally broken and really difficult to reign in since it has a very fuzzy ownership model for ida. I think its extremely unlikely that anyone depends on it. If someone really insists on keeping JS_EnumerateResolvedStandardClasses, I am happy to take a patch. Again, in our codebase its unused.

As for JS_Enumerate, this patch tracks the lifetime between JS_Enumerate and JS_DestroyIdArray, which should remove the GC hazard.

Comment 5

[Andreas Gal :gal]

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

API removal, better ask Brendan for his ok.

Comment 6

[Andreas Gal :gal]

Note: for FF4 and branches it is also an option to just keep the old JS_EnumerateResolvedStandardClasses API and not fix it. We are not using it. Nobody else is. And that one extension using it (seriously unlikely) ... is just out of luck.

Comment 7

[Brendan Eich [:brendan]]

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

Want more noise when the idArraysHandedOutViaAPI underflows or has a stuck number of elements across inactive cx GCs. r=me with that.

/be

Comment 8

[Brendan Eich [:brendan]]

(In reply to comment #6)
> Note: for FF4 and branches it is also an option to just keep the old
> JS_EnumerateResolvedStandardClasses API and not fix it. We are not using it.
> Nobody else is. And that one extension using it (seriously unlikely) ... is
> just out of luck.

It is a useless API. Let's back off and nuke the site from orbit... it's the only way to be sure.

/be

Comment 9

[Andreas Gal :gal]

I was thinking the exact same thing when writing the patch. The vector is stored in the thread, so I can't check during GC because threads without a pending request do not participate in the GC protocol. And as for underflow, I can't easily check for that either because JS_DestroyIdArray is called if RegisterIdArray fails, in which case ... well the ida isn't registered. I could split out the guts of DestroyIdArray and stick the check in JS_DestroyIdArray and call DestroyIdArray internally I guess. Will do that.

Comment 10

[Brendan Eich [:brendan]]

Do what you can to add checks, otherwise we'll have less pleasant diagnostic fun.

It's possible (IIRC) to enumerate threads in a runtime during a full GC.

/be

Comment 11

[Andreas Gal :gal]

http://hg.mozilla.org/tracemonkey/rev/bd821ea0ad41

Comment 12

[Andreas Gal :gal]

Sheila, I think this patch will fix a bunch of crashes with MarkId in the signature.

Comment 13

[Chris Leary [:cdleary] (not checking bugmail)]

http://hg.mozilla.org/mozilla-central/rev/bd821ea0ad41

Comment 14

[Andreas Gal :gal]

This got backed-out from TM.

Comment 15

[Chris Leary [:cdleary] (not checking bugmail)]

(In reply to comment #14)
> This got backed-out from TM.

Please remove FITM from the whiteboard on backouts.

Comment 16

[Andreas Gal :gal]

yeah, I forgot to do that, obviously

Comment 17

[Andrew McCreight [:mccr8]]

JS_Enumerate still doesn't root the JSIDArray it returns, but every user of it in the tree immediately roots the result using AutoIDArray.  Presumably the exact rooting static analysis will detect if somebody forgets to root things, so I think this is okay to close.