Closed Bug 884362 Opened 7 years ago Closed 7 years ago

GC: Fix test crashes in exact browser builds

Categories

(Core :: JavaScript Engine, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla24

People

(Reporter: jonco, Assigned: bholley)

References

Details

Attachments

(4 files, 4 obsolete files)

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.
bholley might be a sensible reviewer for both of these
Attachment #764196 - Flags: review?(bobbyholley+bmo)
Attachment #764198 - Flags: review?(bobbyholley+bmo)
Attachment #764850 - Flags: review?(bzbarsky)
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.
Attachment #764196 - Attachment is obsolete: true
Attachment #764196 - Flags: review?(bobbyholley+bmo)
Assignee: jcoppeard → bobbyholley+bmo
Attachment #764850 - Flags: review?(bzbarsky)
Attachment #764198 - Attachment is obsolete: true
Attachment #764198 - Flags: review?(bobbyholley+bmo)
Attachment #764850 - Attachment is obsolete: true
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.
Attachment #764857 - Flags: review?(bzbarsky)
Attachment #764859 - Flags: review?(bzbarsky)
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
Attachment #764852 - Flags: review?(terrence) → review+
Blocks: 883450
Attachment #764857 - Flags: review?(bzbarsky)
Attachment #764858 - Flags: review?(bent.mozilla)
Attachment #764857 - Flags: review?(bzbarsky)
Attachment #764858 - Flags: review?(bent.mozilla)
Comment on attachment 764857 [details] [diff] [review]
Use AutoJSContext in nsScriptSecurityManager::LoadPolicy. v1

r=me
Attachment #764857 - Flags: review?(bzbarsky) → review+
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.
Attachment #764859 - Flags: review?(bzbarsky) → review+
(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...
Attachment #764859 - Attachment is obsolete: true
Attachment #765034 - Flags: review?(bzbarsky)
Comment on attachment 765034 [details] [diff] [review]
Play Maybe<> games to root |temp| in the right circumstances. v1

r=me
Attachment #765034 - Flags: review?(bzbarsky) → review+
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?
Attachment #764858 - Flags: review?(bent.mozilla) → review+
You need to log in before you can comment on or make changes to this bug.