Closed Bug 1299934 Opened 4 years ago Closed 4 years ago

Run shutdown CCs on workers

Categories

(Core :: DOM: Workers, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox49 --- wontfix
firefox-esr45 --- wontfix
firefox50 --- fixed
firefox51 --- fixed

People

(Reporter: mccr8, Assigned: mccr8)

References

Details

(Whiteboard: [MemShrink])

Attachments

(1 file)

We don't run shutdown GC+CCs in nsCycleCollector::Shutdown() except when we care about leak checking. This makes sense in the main thread, because we're exiting the process right after, but Boris pointed out this makes no sense in DOM workers, because of course the process continues to exist! I don't know how we didn't think about this before.

We are thus probably leaking objects created in workers that we don't in debug builds. The only saving grace is that we don't have many CCed things in workers, and we probably do shutdown GCs anyways which may help.

Forcing shutdown collection in workers should be easy enough. Hopefully it will not cause performance issues.
See Also: → 1299887
I wrote a patch that disables the shutdown CC on workers in debug builds, to simulate the sorts of leaks we might see. I also had to comment out a bunch of JS engine leak checking asserts. Anyways, unsurprisingly, not doing the shutdown collection does cause leaks. Running dom/push/test/ we leak something like 60kb of things like callbacks, DETH, ELMs, various service worker structures.

try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=375fc1ceae7a
Whiteboard: [MemShrink]
Nathan, could you review the XPCOMInit.cpp changes? It is gross to dump this code into XPCOM shutdown, but conceptually I feel like it makes sense to make it the decision of the caller of nsCycleCollector_shutdown() to decide if the shutdown GC/CC should be run. If that's not okay, I can tuck it into nsCycleCollector_shutdown() and do an NS_IsMainThread() check to decide what to do.

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2e2410768ddf
(Collection gets run unconditionally on workers without any changes to worker code because the default argument is is true.)
We should probably backport this, as it feels fairly low risk and the leaks might be bad, if people use sites that create and destroy a lot of workers.
Comment on attachment 8790778 [details]
Bug 1299934 - Run shutdown collection in workers in opt builds.

https://reviewboard.mozilla.org/r/78438/#review76976

r=me on the `xpcom/build/` changes.

::: xpcom/build/XPCOMInit.cpp:991
(Diff revision 1)
>  
> -  nsCycleCollector_shutdown();
> +  bool shutdownCollect;
> +#ifdef NS_FREE_PERMANENT_DATA
> +  shutdownCollect = true;
> +#else
> +  shutdownCollect = PR_GetEnv("MOZ_CC_RUN_DURING_SHUTDOWN");

I feel like we should insert an explicit conversion to bool here, trying to make it obvious that we don't really care what the value of this variable is?

::: xpcom/build/XPCOMInit.cpp:993
(Diff revision 1)
> +#ifdef NS_FREE_PERMANENT_DATA
> +  shutdownCollect = true;
> +#else
> +  shutdownCollect = PR_GetEnv("MOZ_CC_RUN_DURING_SHUTDOWN");
> +#endif
> +  nsCycleCollector_shutdown(shutdownCollect);

I think your reasoning makes sense.  I wish that the parameter wasn't a bool, but an enum:

```
enum ShutdownCollectionBehavior {
  RunGCAndCCs,
  DoNothing,
};
```
Attachment #8790778 - Flags: review?(nfroyd) → review+
(In reply to Nathan Froyd [:froydnj] from comment #6)
> I feel like we should insert an explicit conversion to bool here, trying to
> make it obvious that we don't really care what the value of this variable is?

Sounds reasonable.

> I think your reasoning makes sense.  I wish that the parameter wasn't a
> bool, but an enum:

I didn't do that because I managed to avoid passing in a literal. ;)
Comment on attachment 8790778 [details]
Bug 1299934 - Run shutdown collection in workers in opt builds.

https://reviewboard.mozilla.org/r/78438/#review76984
Attachment #8790778 - Flags: review?(bugs) → review+
Pushed by amccreight@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0154cd832126
Run shutdown collection in workers in opt builds. r=froydnj,smaug
https://hg.mozilla.org/mozilla-central/rev/0154cd832126
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Comment on attachment 8790778 [details]
Bug 1299934 - Run shutdown collection in workers in opt builds.

Approval Request Comment
[Feature/regressing bug #]: CCed stuff in workers (a year or two old)
[User impact if declined]: leaks when using DOM workers
[Describe test coverage new/current, TreeHerder]: this runs all the time
[Risks and why]: Should be low. This makes our DOM worker behavior in release builds match the behavior in our debug builds.
[String/UUID change made/needed]: none
Attachment #8790778 - Flags: approval-mozilla-aurora?
Comment on attachment 8790778 [details]
Bug 1299934 - Run shutdown collection in workers in opt builds.

Makes sense, has stabilized on Nightly for 2 days, Aurora50+
Attachment #8790778 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.