Closed Bug 1542800 Opened 6 months ago Closed 6 months ago

Avoid leaking tasks in the APZUpdater queue on shutdown

Categories

(Core :: Panning and Zooming, defect, P3)

defect

Tracking

()

RESOLVED FIXED
mozilla68
Tracking Status
firefox67 --- fixed
firefox68 --- fixed

People

(Reporter: kats, Assigned: kats)

References

Details

Attachments

(1 file)

Spinoff from bug 1465658 comment 44. I'm fairly sure the patch I have will fix all instances of bug 1465658 that leak APZUpdater instances, but I'm doing it in a dep bug in case there are other factors in play here.

To summarize, my investigation revealed that while a window is shutting down, it's possible for the "root pipeline" to get unset in WR while the content pipeline is still sending display lists. When this happens, WR stops doing scene builds (because of this check which means APZUpdater stops getting epoch update notifications via CompleteSceneSwap. So the UpdateHitTestingTree tasks corresponding to the content pipeline display lists never get processed.

In terms of correctness this fine because the scene was never built and so the updated hit-testing information isn't needed, but it does result in those tasks getting leaked. As the tasks hold a RefPtr to the enclosing APZUpdater, that APZUpdater never gets cleaned up, and it can entrain other objects into getting leaked as well.

This patch ensures that once the ClearTree task is run on the updater
thread, we throw away any other remaining tasks in the queue, as they
will never get run. (Note that the ClearTree task removes the APZUpdater
instance from the global map, and so it becomes "unfindable", meaning
the ProcessQueue will never run again on that instance).

Pushed by kgupta@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f7514ce0ff13
Clear out any leftover updater tasks after the ClearTree task is run. r=botond
Status: NEW → RESOLVED
Closed: 6 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla68

This is probably worth uplifting.

Comment on attachment 9056590 [details]
Bug 1542800 - Clear out any leftover updater tasks after the ClearTree task is run. r?botond

Beta/Release Uplift Approval Request

  • Feature/Bug causing the regression: Bug 1449982
  • User impact if declined: Closing a window with WR enabled has a chance that it will leak a bunch of objects in the GPU process.
  • Is this code covered by automated tests?: Yes
  • Has the fix been verified in Nightly?: Yes
  • Needs manual test from QE?: No
  • If yes, steps to reproduce: No concrete STR but we were hitting this as an intermittent failure in bug 1465658. Since this patch landed there have been no instances of it, so I take that as a good sign.
  • List of other uplifts needed: None
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): It's a pretty low risk patch given the architecture of the code. It just causes some leftover messages in a queue to get discarded after the queue processor is shut down.
  • String changes made/needed:
Attachment #9056590 - Flags: approval-mozilla-beta?

Comment on attachment 9056590 [details]
Bug 1542800 - Clear out any leftover updater tasks after the ClearTree task is run. r?botond

Low risk with tests and in the context of WR that we start shipping in 67, uplift approved for 67 beta 11, thanks.

Attachment #9056590 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

I tried to uplift this but got a conflict

mozilla@ubuntu ~/mozilla-unified beta(+4) $ hg graft -er f7514ce0ff13
grafting 534588:f7514ce0ff13 "Bug 1542800 - Clear out any leftover updater tasks after the ClearTree task is run. r=botond"
merging gfx/layers/apz/public/APZUpdater.h
merging gfx/layers/apz/src/APZUpdater.cpp
warning: conflicts while merging gfx/layers/apz/src/APZUpdater.cpp! (edit, then use 'hg resolve --mark')

Flags: needinfo?(kats)

I can uplift, thanks for the heads up.

Flags: needinfo?(kats)
No longer blocks: 1465658
Duplicate of this bug: 1465658
You need to log in before you can comment on or make changes to this bug.