1005378 - TabChild can give its FrameMetrics a bogus scroll id

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

[Botond Ballo [:botond]]

In HandlePossibleViewportChange(), TabChild queries the scroll identifiers associated with the document element [1]. The scroll identifiers include the view id (a.k.a. "scroll id") and the pres shell id. The view id may not have been set yet, in which case APZCCallbackHelper::GetScrollIdentifiers() returns false, and the 'viewId' variable in HPVC() remains uninitialized.

However, on the first paint, we unconditionally set 'viewId' [2] as the scroll id of the FrameMetrics object which is later saved as field of TabChild [3].

Therefore, if the document element has not yet been assigned a view id when TabChild's before-first-paint handler is called, TabChild will store a FrameMetrics with a bogus scroll id.

This is what seems to be happening on the first paint for a mochitest run in b2g-emulator, and it's causing me problems for bug 961289.

It's not immediately clear to me what we should be doing in TabChild::HPVC() when the scroll identifiers are not valid - possibly just exiting early, though the fact that some but not all code in HPVC() is conditioned on the identifiers being valid suggests otherwise.

[1] http://dxr.mozilla.org/mozilla-central/source/dom/ipc/TabChild.cpp?from=TabChild.cpp#199
[2] http://dxr.mozilla.org/mozilla-central/source/dom/ipc/TabChild.cpp?from=TabChild.cpp#288
[3] http://dxr.mozilla.org/mozilla-central/source/dom/ipc/TabChild.cpp?from=TabChild.cpp#322

Comment 1

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

We should be able to change the FindIDFor call in GetScrollIdentifiers to a FindOrCreateIDFor. As long as the document element exists (which it does if we're at first paint) we might as well create the id earlier.

Comment 2

[Botond Ballo [:botond]]

I did some investigation to see why we hadn't run into this so far, with our routine use/testing of device builds. It turns out that this assignment of a bogus scroll id does happen on device builds as well, but shortly afterwards (and in particular, after a scroll id has been assigned in nsDisplayList::PaintForFrame()), there's invariably a call to TabChild::RecvUpdateDimensions() which calls HandlePossibleViewportChange() again and this time picks up the correct scroll id.

Whether or not such a call to RecvUpdateDimensions() is expected on emulator builds, I don't think we should rely on it happening, so I think the solution Kats suggested in comment #1 is reasonable.

Comment 3

[Botond Ballo [:botond]]

Attachment: bug1005378.patch

This patch implements Kats' suggestion.

Comment 4

[Botond Ballo [:botond]]

https://tbpl.mozilla.org/?tree=Try&rev=5f15e963dde7

Comment 5

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1170c75f8a87