955735 - Re-organize the code in nsScriptLoader to eliminate the last two hazards

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Terrence Cole [:terrence]]

Attachment: hazards_nsScriptLoader-v0.diff

The problem here is that we get the ScriptContext and the JS GlobalObject at once since the lookup of the nsIScriptGlobalObject is costly. This makes the JS object live across a bunch of stuff that either may GC, or is hard to make the analysis understand.

However, it looks like the lookup of the JSObject and the ScriptContext off the nsIScriptGlobalObject are trivial, so if we just move the nsIScriptGlobalObject up a level, all our order-of-operations problems can just go away.

This should be the last two exact rooting hazards. Try build is at:
https://tbpl.mozilla.org/?tree=Try&rev=0930152dfc87

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8354903 [details] [diff] [review]
hazards_nsScriptLoader-v0.diff


>+nsIScriptGlobalObject *
* goes with the type
>+nsScriptLoader::GetScriptGlobalObject()
> {
>   nsPIDOMWindow *pwin = mDocument->GetInnerWindow();
>   if (!pwin) {
>     return nullptr;
>   }
> 
>   nsCOMPtr<nsIScriptGlobalObject> globalObject = do_QueryInterface(pwin);
>   NS_ASSERTION(globalObject, "windows must be global objects");
> 
>   // and make sure we are setup for this type of script.
>   nsresult rv = globalObject->EnsureScriptEnvironment();
>   if (NS_FAILED(rv)) {
>     return nullptr;
>   }
> 
>-  *aGlobal = globalObject->GetGlobalJSObject();
>+  return globalObject;
>+}
Please make this method return already_AddRefed<nsIScriptGlobalObject>
and then return globalObject.forget();


>+nsIScriptContext *
>+nsScriptLoader::GetScriptContext(nsIScriptGlobalObject *globalObject)
>+{
>   return globalObject->GetScriptContext();
> }
> 
>+JSObject *
>+nsScriptLoader::GetScriptJSGlobalObject(nsIScriptGlobalObject *globalObject)
>+{
>+  return globalObject->GetGlobalJSObject();
>+}
Why we need these two methods?
(and * goes with type and params should be in form aFoo)

Comment 2

[Terrence Cole [:terrence]]

Attachment: hazards_nsScriptLoader-v1.diff

Gah! I have no idea how that extraneous hunk slipped in. I thought I cut the patch /after/ building and pushing to try, but apparently not. In any case, the try run (which doesn't have the hunk) is green.

Comment 3

[Terrence Cole [:terrence]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/084c49c8db9e

Comment 4

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/084c49c8db9e