884362 - GC: Fix test crashes in exact browser builds

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jon Coppeard (:jonco)]

There are currently many failures running a try build with exact rooting turned on, although I think these are all caused by a few underlying problems.

Comment 1

[Jon Coppeard (:jonco)]

Attachment: Fix crash in nsScriptSecurityManager::LookupPolicy

Comment 2

[Jon Coppeard (:jonco)]

Attachment: Fix crash in IDBFactory::OpenInternal

Comment 3

[Andrew McCreight [:mccr8]]

bholley might be a sensible reviewer for both of these

Comment 4

[Jon Coppeard (:jonco)]

Attachment: Fix crash in WebGLRenderingContextBinding.cpp

Comment 5

[Jon Coppeard (:jonco)]

Attachment: Assert Rooted is constructed with non-null argument in all builds

Comment 6

[Bobby Holley (:bholley)]

Comment on attachment 764196 [details] [diff] [review]
Fix crash in nsScriptSecurityManager::LookupPolicy

So, I've actually got more in-depth patches for both issues. I'll post.

Comment 7

[Bobby Holley (:bholley)]

Attachment: Use AutoJSContext in nsScriptSecurityManager::LoadPolicy. v1

This cx is just used for rooting etc. As such, we really just want whatever
cx is on the stack, but want a default if the stack-top cx is null (which it
sometimes is, here). And that's exactly what AutoJSContext gives us.

Comment 8

[Bobby Holley (:bholley)]

Attachment: Make IDBRequest::CaptureCaller use the cx stack and decxify a bunch of IDB. v1

Comment 9

[Bobby Holley (:bholley)]

Attachment: Use the SafeJSContext to root |temp| for nullables. v1

Comment 10

[Terrence Cole [:terrence]]

Comment on attachment 764852 [details] [diff] [review]
Assert Rooted is constructed with non-null argument in all builds

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

r=me

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 764857 [details] [diff] [review]
Use AutoJSContext in nsScriptSecurityManager::LoadPolicy. v1

r=me

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 764859 [details] [diff] [review]
Use the SafeJSContext to root |temp| for nullables. v1

Hmm.  This rather sucks, because in fact if !cx then temp needs no rooting: it's always undefined.  :(

r=me, for lack of better options.   I suppose we could play games with Maybe<Rooted>, but it's not worth it.

Comment 13

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky (:bz) from comment #12)
> r=me, for lack of better options.   I suppose we could play games with
> Maybe<Rooted>, but it's not worth it.

Ugh - this stuff actually gets called on workers as well, in which case we can't use the SafeJSContext(). Time to play some Maybe<> games...

Comment 14

[Bobby Holley (:bholley)]

Attachment: Play Maybe<> games to root |temp| in the right circumstances. v1

Comment 15

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 765034 [details] [diff] [review]
Play Maybe<> games to root |temp| in the right circumstances. v1

r=me

Comment 16

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

Comment on attachment 764858 [details] [diff] [review]
Make IDBRequest::CaptureCaller use the cx stack and decxify a bunch of IDB. v1

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

r=me with these addressed:

::: dom/indexedDB/IDBFactory.cpp
@@ +529,5 @@
>                           IDBOpenDBRequest** _retval)
>  {
>    NS_ASSERTION(NS_IsMainThread(), "Wrong thread!");
>    NS_ASSERTION(mWindow || mOwningObject, "Must have one of these!");
> +  AutoJSContext cx;

Nit: newline between opening assertions and this variable, here and in several other places in this patch.

::: dom/indexedDB/IDBRequest.cpp
@@ +209,4 @@
>  
>    const char* filename = nullptr;
>    uint32_t lineNo = 0;
> +  if (!nsJSUtils::GetCallingLocation(cx, &filename, &lineNo)) {

Can you keep the warning in the case where we aren't using the safe context?

Comment 17

[Bobby Holley (:bholley)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/424742645e2e
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/1d1e5e39c768
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/8051c24ed9d8

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/424742645e2e
https://hg.mozilla.org/mozilla-central/rev/1d1e5e39c768
https://hg.mozilla.org/mozilla-central/rev/8051c24ed9d8