974327 - Cleanup nsJSContext::GetGlobalObject()

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Patch v1

If mGlobalObjectRef is null, we would non-fatally assert and then return null because ((~c->flags) & (JSCLASS_HAS_PRIVATE | JSCLASS_PRIVATE_IS_NSISUPPORTS)) is true.

The assertion isn't hit by our linux64 debug tests: <https://tbpl.mozilla.org/?tree=Try&rev=7991ead81f7a>. Not sure how useful it is.

Comment 1

[Bobby Holley (:bholley)]

Comment on attachment 8378203 [details] [diff] [review]
Patch v1

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

r=me with that. Thanks for doing this.

::: dom/base/nsJSEnvironment.cpp
@@ +1070,5 @@
>      return nullptr;
>    }
>  
> +  MOZ_ASSERT(mGlobalObjectRef);
> +  return mGlobalObjectRef;

How about just:

MOZ_ASSERT_IF(mWindowProxy, mGlobalObjectRef);
return mGlobalObjectRef?

Comment 2

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

(In reply to Bobby Holley (:bholley) from comment #1)
> Comment on attachment 8378203 [details] [diff] [review]
> Patch v1
> 
> Review of attachment 8378203 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me with that. Thanks for doing this.
> 
> ::: dom/base/nsJSEnvironment.cpp
> @@ +1070,5 @@
> >      return nullptr;
> >    }
> >  
> > +  MOZ_ASSERT(mGlobalObjectRef);
> > +  return mGlobalObjectRef;
> 
> How about just:
> 
> MOZ_ASSERT_IF(mWindowProxy, mGlobalObjectRef);
> return mGlobalObjectRef?

Is that supposed to be equivalent?

Comment 3

[Bobby Holley (:bholley)]

(In reply to :Ms2ger from comment #2)
> Is that supposed to be equivalent?

Not strictly, no. But the lifetimes are very similar - both are nulled out at the same time. And the only time mWindowProxy is null and mGlobalObjectRef is not is between the initial construction of an nsJSContext (in nsGlobalWindow::EnsureScriptEnvironment()) and the first call to nsGlobalWindow::SetNewDocument (generally during the creation of an about:blank content viewer). And I don't see any good reason to return null during that window.

Comment 4

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Looks like this got lost... Bobby, are you okay with me landing the patch as-in? I'd rather not deal with any fallout if it turns out something did depend on the null return there.

Comment 5

[Bobby Holley (:bholley)]

Sure. But at least add a comment around the affected code indicating that it can probably be simplified further, and pointing to comment 1 and comment 3?

Comment 6

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b75c7cc9972a

Comment 7

[Ed Morley [:emorley]]

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