1207270 - Leaks causing Linux M-e10s-4 failures with APZ enabled

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

[Kartikaya Gupta (email:[e-mail])]

With APZ enabled, the Linux M-e10s-4 tests are failing due to leaks. I'm looking into it.

Comment 1

[Kartikaya Gupta (email:[e-mail])]

This appears to be because the TaskThrottler instance is created on the compositor thread, and so the mTimer object is also created on the compositor thread. The nsITimer documentation says that tasks are run on the thread that the timer object is created on. Therefore, TaskThrottler timer tasks (which were introduced in bug 1200063) are scheduled onto the compositor thread, but are never actually run because I believe the compositor thread isn't an nsITimer-compatible thread. Instead they get leaked or something, and hold on to various objects. I put in a hack locally to force the TaskThrottler instances to be created on the main thread and that fixed the problem for me locally.

Comment 2

[Kartikaya Gupta (email:[e-mail])]

Also for future reference this is the magical command that I found most helpful to debug the issue:

./mach mochitest -f plain --e10s --setenv XPCOM_MEM_REFCNT_LOG=/tmp/leaks.log --setenv XPCOM_MEM_LOG_CLASSES=nsTimerImpl --debugger=rr --setpref layers.async-pan-zoom.enabled=true gfx/layers/apz/test 2>&1 | tee leaks.log

(rr replay will then let you repro the leak on demand, and you can use the output from leaks.log to find the leaked nsTimerImpl object and follow it around).

Comment 3

[Kartikaya Gupta (email:[e-mail])]

Attachment: Part 1 - Ensure TaskThrottlers are created on the main thread

Comment 4

[Kartikaya Gupta (email:[e-mail])]

Attachment: Part 2 - Update tests

Comment 5

[Kartikaya Gupta (email:[e-mail])]

The try push at https://treeherder.mozilla.org/#/jobs?repo=try&revision=4384db0b25e3&group_state=expanded which has these patches and APZ enabled is showing some M-bc failures which are caused by these patches. I'm investigating.

Comment 6

[Kartikaya Gupta (email:[e-mail])]

Attachment: Part 1 - Ensure TaskThrottlers are created on the main thread

Updated to handle tabs getting dragged from one window to another. In this case we need to move the TaskThrottler over from the old APZCTM to the new one.

Comment 7

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8665101 [details] [diff] [review]
Part 1 - Ensure TaskThrottlers are created on the main thread

The other idea I had for implementing this was to stick the TaskThrottler on the LayerTreeState object and create it at the same time the GeckoContentController is assigned, and access it from APZCTreeManager the same way the GCC is accessed. However that means we have to expose the TaskThrottler outside the apz/ codewhich I kind of wanted to avoid. It might have ended up cleaner; I can try it if you think you'd prefer that.

Comment 8

[Botond Ballo [:botond]]

Comment on attachment 8665101 [details] [diff] [review]
Part 1 - Ensure TaskThrottlers are created on the main thread

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

I think what might be nice to do in the future is to group GeckoContentController, TaskThrottler, and APZTestData into "a per-{layers id} APZ state" structure in LayerTreeState. This approach is fine for now, though.

::: gfx/layers/apz/src/APZCTreeManager.cpp
@@ +222,5 @@
> +  if (aOldManager == this) {
> +    return;
> +  }
> +  mPaintThrottlerMap[aLayersId] = aOldManager->mPaintThrottlerMap[aLayersId];
> +  aOldManager->mPaintThrottlerMap.erase(aLayersId);

Better:

auto iter = aOldManager->mPaintThrottlerMap.find(aLayersId);
if (iter != aOldManager->mPaintThrottlerMap.end()) {
  mPaintThrottlerMap[aLayersId] = iter->second;
  aOldManager->mPaintThrottlerMap.erase(iter);
}

This avoids a second lookup in aOldManager->mPaintThrottlerMap (as we're calling 'erase(iterator)' instead of 'erase(key)'), and also avoids gratuitously inserting and erasing a null value (and leaving this->mPaintThrottlerMap with it) if aLayersId wasn't present.

On a different note: should we cancel any pending task the trasnferred TaskThrottler might have?

Comment 9

[Kartikaya Gupta (email:[e-mail])]

(In reply to Botond Ballo [:botond] from comment #8)
> This avoids a second lookup in aOldManager->mPaintThrottlerMap (as we're
> calling 'erase(iterator)' instead of 'erase(key)'), and also avoids
> gratuitously inserting and erasing a null value (and leaving
> this->mPaintThrottlerMap with it) if aLayersId wasn't present.

Ah, good point. I'll update it as you suggested.

> On a different note: should we cancel any pending task the trasnferred
> TaskThrottler might have?

I'm not 100% sure, but I'm leaning towards no. If a tab is moved from one APZCTM to another while it has a pending repaint we probably still want to do that repaint. The GeckoContentController associated with the tab is still the same as it was before, so firing a repaint request should still work correctly after the switch. That being said, when this happens the APZC instances themselves will probably get destroyed and re-recreated in a different compositor so most likely what will happen is that the pending task will be a no-op and a new repaint will be triggered regardless.

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8a5c82500dcb
https://hg.mozilla.org/integration/mozilla-inbound/rev/863fd47ccd31

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8a5c82500dcb
https://hg.mozilla.org/mozilla-central/rev/863fd47ccd31