Uninitialised uses of TabChildBase::mScrolling at FxOS startup

RESOLVED FIXED in Firefox 39



Panning and Zooming
3 years ago
3 years ago


(Reporter: jseward, Assigned: kats)


Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

(firefox39 fixed)



(3 attachments, 2 obsolete attachments)



3 years ago
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

3 years ago
Created attachment 8560544 [details]
Valgrind complaint (one of several)
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.
Assignee: nobody → bugmail.mozilla

Comment 3

3 years ago
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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.
Created attachment 8562326 [details] [diff] [review]

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
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.
Depends on: 1135099
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.
Depends on: 1132371
Created attachment 8567176 [details] [diff] [review]
WIP v2

New try push at https://treeherder.mozilla.org/#/jobs?repo=try&revision=b8bf2ef57714
Attachment #8562326 - Attachment is obsolete: true
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.
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.
Created attachment 8567725 [details] [diff] [review]
Part 1 - Remove the mozasyncpanzoom attribute

Try push with just this patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=60c837dff9f2
Attachment #8567725 - Flags: review?(roc)
Created attachment 8567726 [details] [diff] [review]
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).
Attachment #8567176 - Attachment is obsolete: true
Attachment #8567726 - Flags: review?(roc)
Attachment #8567726 - Flags: review?(fabrice)
And another try push rebased on m-c including mochitests, just for good measure:

Attachment #8567725 - Flags: review?(roc) → review+
Attachment #8567726 - Flags: review?(roc) → review+
Attachment #8567726 - Flags: review?(fabrice) → review+
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/4e8d7ee76f05
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/5ac8bf7b8ee3
Last Resolved: 3 years ago
status-firefox39: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
You need to log in before you can comment on or make changes to this bug.