Make nsMessageManagerScriptExecutor::mGlobal into a regular heap pointer

RESOLVED FIXED in Firefox 55

Status

()

Core
DOM
RESOLVED FIXED
5 months ago
5 months ago

People

(Reporter: mccr8, Assigned: mccr8)

Tracking

unspecified
mozilla55
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments)

(Assignee)

Description

5 months ago
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 4

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

Comment 5

5 months ago
mozreview-review
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.
Attachment #8868266 - Flags: review?(bugs) → review+

Comment 6

5 months ago
mozreview-review
Comment on attachment 8868267 [details]
Bug 1365086, part 2 - Add unlink method for nsMessageManagerScriptExecutor.

https://reviewboard.mozilla.org/r/139852/#review143212
Attachment #8868267 - Flags: review?(bugs) → review+
(Assignee)

Comment 7

5 months ago
(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

5 months ago
mozreview-review
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.
Attachment #8868268 - Flags: review?(bugs) → review+

Comment 9

5 months ago
Pushed by amccreight@mozilla.com:
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

5 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/2ca54e1cbf46
https://hg.mozilla.org/mozilla-central/rev/61d7468cd1c1
https://hg.mozilla.org/mozilla-central/rev/d4e14b275a0f
Status: NEW → RESOLVED
Last Resolved: 5 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.