1365086 - Make nsMessageManagerScriptExecutor::mGlobal into a regular heap pointer

Status: RESOLVED FIXED

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

Description

[Andrew McCreight [:mccr8]]

Right now it has type nsCOMPtr<nsIXPConnectJSObjectHolder>, which is archaic. This class is already cycle collected, so it is not be too hard to convert.

Comment 1

[Andrew McCreight [:mccr8]]

Attachment: Bug 1365086, part 1 - Make nsMessageManagerScriptExecutor::GetGlobal return a JSObject pointer.

Review commit: https://reviewboard.mozilla.org/r/139850/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/139850/

Comment 2

[Andrew McCreight [:mccr8]]

Attachment: Bug 1365086, part 2 - Add unlink method for nsMessageManagerScriptExecutor.

Review commit: https://reviewboard.mozilla.org/r/139852/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/139852/

Comment 3

[Andrew McCreight [:mccr8]]

Attachment: Bug 1365086, part 3 - Make nsMessageManagerScriptExecutor::mGlobal into a raw pointer.

Review commit: https://reviewboard.mozilla.org/r/139854/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/139854/

Comment 4

[Andrew McCreight [:mccr8]]

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=90778283f2ef1313ce31b6881d0848982b2ad5d0

Comment 5

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

Comment on attachment 8868266 [details]
Bug 1365086, part 1 - Make nsMessageManagerScriptExecutor::GetGlobal return a JSObject pointer.

https://reviewboard.mozilla.org/r/139850/#review143210

::: dom/base/nsFrameMessageManager.h:370
(Diff revision 1)
>  public:
>    static void PurgeCache();
>    static void Shutdown();
> -  already_AddRefed<nsIXPConnectJSObjectHolder> GetGlobal()
> +  JSObject* GetGlobal()
>    {
> -    nsCOMPtr<nsIXPConnectJSObjectHolder> ref = mGlobal;
> +    return mGlobal->GetJSObject();

Is it guaranteed mGlobal is non-null here?
I'd prefer a null check.

Comment 6

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

Comment on attachment 8868267 [details]
Bug 1365086, part 2 - Add unlink method for nsMessageManagerScriptExecutor.

https://reviewboard.mozilla.org/r/139852/#review143212

Comment 7

[Andrew McCreight [:mccr8]]

(In reply to Olli Pettay [:smaug] from comment #5)
> Is it guaranteed mGlobal is non-null here?
> I'd prefer a null check.

That actually just goes away in a later patch. (mGlobal becomes a JSObject, and we just return it directly.)

Comment 8

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

Comment on attachment 8868268 [details]
Bug 1365086, part 3 - Make nsMessageManagerScriptExecutor::mGlobal into a raw pointer.

https://reviewboard.mozilla.org/r/139854/#review143214

Given that this changes black root marking a bit (XPCJSRuntime::TraceNativeBlackRoots), worth to check whether telemetry data around CC changes.

::: dom/base/nsFrameMessageManager.h:370
(Diff revision 1)
>  public:
>    static void PurgeCache();
>    static void Shutdown();
>    JSObject* GetGlobal()
>    {
> -    return mGlobal->GetJSObject();
> +    return mGlobal;

Ah, we change mGlobal here, so null check isn't needed.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2ca54e1cbf46
part 1 - Make nsMessageManagerScriptExecutor::GetGlobal return a JSObject pointer. r=smaug
https://hg.mozilla.org/integration/autoland/rev/61d7468cd1c1
part 2 - Add unlink method for nsMessageManagerScriptExecutor. r=smaug
https://hg.mozilla.org/integration/autoland/rev/d4e14b275a0f
part 3 - Make nsMessageManagerScriptExecutor::mGlobal into a raw pointer. r=smaug

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2ca54e1cbf46
https://hg.mozilla.org/mozilla-central/rev/61d7468cd1c1
https://hg.mozilla.org/mozilla-central/rev/d4e14b275a0f