1130455 - Uninitialised uses of TabChildBase::mScrolling at FxOS startup

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

[Julian Seward [:jseward]]

Observed on every startup of FxOS, on both Flame and Nexus5.

It appears that TabChild::IsAsyncPanZoomEnabled() is called in 
quite a number of places, and that inspects TabChildBase::mScrolling.

But TabChildBase::TabChildBase() doesn't set mScrolling to anything.
Maybe it is supposed to be set later on?

Comment 1

[Julian Seward [:jseward]]

Attachment: Valgrind complaint (one of several)

Comment 2

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

I can take this. It would be nice to get rid of TabChild::IsAsyncPanZoomEnabled completely, along with the mozasyncpanzoom attribute. We should be able to just use the pref instead. I can try to do that.

Comment 3

[Julian Seward [:jseward]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #2)
> I can take this.

That would be good.  I can test any patch(es) you have.  The uninitialised
use of TabChildBase::mScrolling happens in quite a number of places during
FxOS startup, so fixing it or removing it entirely would significantly
improve the Valgrind-cleanness of FxOS startup.

Comment 4

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

Attachment: WIP

First crack at this. After poking around the code a bit I don't think we can safely get rid of the mozasyncpanzoom attribute entirely because it's used in reftests [1], presumably to make the child content process have APZ enabled even though the parent harness-runner process doesn't.

Still, TabChild already inherits from TabContext which has a mScrollingBehavior that should be set correctly on initialization so I don't see the point of having a separate mScrolling field that basically does the same thing. This patch removes it and uses the mScrollingBehavior directly.

Try push at https://treeherder.mozilla.org/#/jobs?repo=try&revision=9251c1417276

[1] http://mxr.mozilla.org/mozilla-central/source/layout/tools/reftest/reftest.js?rev=6084da03dcd8#274

Comment 5

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

The try push above shows a few failures in R-e10s. I can reproduce those failures locally but interestingly enough I get those failures even without this patch. I'm pretty sure I'm running the test correctly. I'll try to debug it although it may take a few days to get around to it.

Comment 6

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

So, I think until now R-e10s was running with APZ $random?enabled:disabled. On automation this generally resolved to "disabled" but locally it resolved to "enabled". Therefore I was seeing these reftest failures locally even without my patch but they don't show up on automation. The patch forces APZ enabled when it's supposed to, instead of being $random, so now the reftests failures show up on automation too.

Two of the failures are already skip-if(B2G) which I think is more correct as skip-if(asyncPanZoom), so I'll update those tests. I filed bug 1135099 for fixing one which actually looks like a bug. The third failure I think will be taken care of by bug 1132371 because it's a color-is-off-by-one failure similar to that.

I'm rebuilding and will re-test locally before doing another try push.

Comment 7

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

Attachment: WIP v2

New try push at https://treeherder.mozilla.org/#/jobs?repo=try&revision=b8bf2ef57714

Comment 8

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

So the try push above still has failures, which is very odd. Specifically one of the failures is bugs/641770-1.html which I explicitly made skip-if(asyncPanZoom). This implies that the reftest harness is wrong and isn't properly detecting if APZ is enabled or not. Either that or APZ is actually disabled, but in that case I have no clue what's going on here. Either way running it locally doesn't replicate the conditions under which it's run on tryserver.

Comment 9

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

Ok so I think the root of this reftest failure is because the reftest harness always sets the mozasyncpanzoom attribute on child processes even if the layers.async-pan-zoom.enabled pref is false. This doesn't work for enabling APZ in the child process any more, and in fact causes the child process to get confused as to whether or not APZ is enabled (even ignoring the uninitialized variables). We can just rip all of that stuff out and simplify this code quite a bit.

Comment 10

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

Attachment: Part 1 - Remove the mozasyncpanzoom attribute

Try push with just this patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=60c837dff9f2

Comment 11

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

Attachment: Part 2 - Remove all the ScrollingBehavior stuff

Try push with part 1 + this patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=05ad08d3689a (this patch is a squash of the last two patches in the try push).

Comment 12

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

And another try push rebased on m-c including mochitests, just for good measure:

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

Comment 13

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

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/4e8d7ee76f05
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/5ac8bf7b8ee3

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/4e8d7ee76f05
https://hg.mozilla.org/mozilla-central/rev/5ac8bf7b8ee3