1124452 - Create an APZC for the root scroll frame of the root document in the parent process

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

[Botond Ballo [:botond]]

Our current logic for which scroll frames gets APZCs is:

  - Any scroll frame gets an APZC if it's overflow:scroll and has
    a non-empty scroll range. (The layerization of the scroll frame
    and the creation of the APZC is generally deferred until the first 
    event is targeted at the frame (modulo an optimization done in
    bug 982141)).

  - As a special case, the root content document's root scroll frame
    (RCD-RSF) always gets an APZC, even if it has an empty scroll
    range or it's overflow:hidden. This ensures that all content in
    the root content document (which on B2G means all content in a
    child process) is covered by an APZC. This is important because
    APZ performs functions such as processing tap gestures, and it
    can only do so for content covered by an APZC.

To enable APZ in the B2G parent process, we'll also need to ensure that an APZC covers all content in the parent process, even if the root scroll frame of the root document in the parent process (PP-RD-RSF) is overflow:hidden or has an empty scroll range. 

This will effectively mean extending the special case mentioned above to the PP-RD-RSF.

Comment 1

[Botond Ballo [:botond]]

The mechanism for ensuring the RCD-RSF has an APZC has changed with containerless scrolling for root scroll frames ("containerless-root").

I believe we now rely on someone to set a displayport on the RCD-RSF (for TabChild, this happens in the ProcessUpdateFrame call in HandlePossibleViewportChange), and the displayport then triggers the creation of scrollable metrics and an APZC.

I propose implementing the creation of APZC for the PP-RD-RSF for containerless-root only. I believe we can do it by calling APZCCallbackHelper::GetOrCreateScrollIdentifiers() and then nsIDOMWindowUtils::SetDisplayPortMarginsForElement from ChromeProcessController.

Comment 2

[Botond Ballo [:botond]]

Attachment: Part 1 - Store the widget in ChromeProcessController

(Originally posted as Part 2 patch in bug 1005815. Moving it here because I need the widget in the next patch.)

Comment 3

[Botond Ballo [:botond]]

Attachment: Part 2 - Set a displayport for the PP-RD-RSF

Timothy, I took a stab at how to get the document element from the widget. Let me know if this is reasonable.

Comment 4

[Botond Ballo [:botond]]

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

Comment 5

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

Comment on attachment 8553937 [details] [diff] [review]
Part 1 - Store the widget in ChromeProcessController

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

::: gfx/layers/apz/util/ChromeProcessController.h
@@ +42,5 @@
>    virtual void HandleLongTapUp(const CSSPoint& aPoint, int32_t aModifiers,
>                                 const ScrollableLayerGuid& aGuid) MOZ_OVERRIDE {}
>    virtual void SendAsyncScrollDOMEvent(bool aIsRoot, const mozilla::CSSRect &aContentRect,
>                                         const mozilla::CSSSize &aScrollableSize) MOZ_OVERRIDE {}
> +private:

nit: add a blank line above private:

Comment 6

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

Comment on attachment 8553940 [details] [diff] [review]
Part 2 - Set a displayport for the PP-RD-RSF

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

My only concern is that this is going to get run pretty early during startup - I hope that there's always a contentdoc here but I don't know enough about startup flow to say for sure.

Comment 7

[Timothy Nikkel (:tnikkel)]

(In reply to Botond Ballo [:botond] from comment #3)
> Created attachment 8553940 [details] [diff] [review]
> Part 2 - Set a displayport for the PP-RD-RSF
> 
> Timothy, I took a stab at how to get the document element from the widget.
> Let me know if this is reasonable.

You have rediscovered the usual way of doing that. :)

Comment 8

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

Attachment: One iteration of the infinite recursion

I get infinite recursion on startup with the part 2 patch, because SetDisplayPortMargins calls GetLayerManager which tries to create a compositor and a new ChromeProcessController.

Comment 9

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

Also it appears that this patch doesn't actually force an APZC to get created for the root layer, because the scrollid for the root ContainerLayer is 0 because of aForceNullScrollId [1].

[1] http://mxr.mozilla.org/mozilla-central/source/layout/base/nsDisplayList.cpp?rev=301bf68c9566#714

Comment 10

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

Attachment: Additional fixup

Additional fixup I needed to do. Feel free to steal/combine into other patches.

Comment 11

[Botond Ballo [:botond]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #8)
> I get infinite recursion on startup with the part 2 patch, because
> SetDisplayPortMargins calls GetLayerManager which tries to create a
> compositor and a new ChromeProcessController.

Good catch! I'm not sure why I didn't get it during testing, but it definitely needs to be fixed.(In reply to Kartikaya Gupta (email:[e-mail]) from comment #9)

> Also it appears that this patch doesn't actually force an APZC to get
> created for the root layer, because the scrollid for the root ContainerLayer
> is 0 because of aForceNullScrollId.

The code path that passes aForceNullScrollId = true [1] only runs without containerless-root. My original plan (see comment 1) was to only support this with containerless-root, but since your patch makes it work without containerless-root, too (and it turned out to pretty easy to do so), let's roll with that :)

[1] http://mxr.mozilla.org/mozilla-central/source/layout/base/nsDisplayList.cpp?rev=301bf68c9566#1626

Comment 12

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

(In reply to Botond Ballo [:botond] from comment #11)
> The code path that passes aForceNullScrollId = true [1] only runs without
> containerless-root. My original plan (see comment 1) was to only support
> this with containerless-root, but since your patch makes it work without
> containerless-root, too (and it turned out to pretty easy to do so), let's
> roll with that :)

Ah, good point. I'm not sure if my patch will have other bad side-effects. I can't think of any at the moment but that doesn't mean much...

Comment 13

[Botond Ballo [:botond]]

Attachment: Part 2 - Store the main thread's MessageLoop in ChromeProcessController

Originally posted as the Part 1 patch in bug 1005815, but I had to move it here because Kats' fix for the infinite recursion needs it. Carrying r+ from there.

Comment 14

[Botond Ballo [:botond]]

Attachment: Part 3 - Set a displayport for the PP-RD-RSF

Now including the fix for the infinite recursion.

Comment 15

[Botond Ballo [:botond]]

Attachment: Part 4 - Make it work without containerless-root, too

This is the rest of Kats' "Additional fixup" patch (the fix for the infinite recursion part was moved into Part 3).

Comment 16

[Botond Ballo [:botond]]

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7d78b44b8746

Comment 17

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

Probably worth checking to make sure part 4 doesn't break Fennec. I don't remember exactly why we had that force-null stuff there to begin with.

Comment 18

[Botond Ballo [:botond]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #17)
> Probably worth checking to make sure part 4 doesn't break Fennec. I don't
> remember exactly why we had that force-null stuff there to begin with.

Another Try push to run Fennec tests: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a6e0043d96f6

Also building locally to test.

Comment 19

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #18)
> Also building locally to test.

Manual testing didn't reveal any obvious breakage.

Comment 20

[Botond Ballo [:botond]]

Looks like we're getting a crash in nsBaseWidget::Release() with these patches.

Comment 21

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

I think the crash is likely because the ChromeProcessController is getting destroyed on the compositor thread, and nsIWidget doesn't have threadsafe addref'ing.

Comment 22

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

I repro'd locally and wrote a patch to clear the mWidget ptr on the main thread instead. Seemed to work locally. Try push with this patch:

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

The patch itself is at https://hg.mozilla.org/try/rev/ec51b5dfb839

Comment 23

[Botond Ballo [:botond]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #22)
> I repro'd locally and wrote a patch to clear the mWidget ptr on the main
> thread instead. Seemed to work locally.

Thanks! I thought the problem might have to do with the fact that ChromeProcessController storing a ref to the widget extends the lifetime of the widget too long, but I failed to consider that it might be changing the _thread_ from which it's destroyed.

Comment 24

[Botond Ballo [:botond]]

Attachment: Part 5 - Ensure the widget continues to be destroyed on the main thread

Posting Kats' fix for the Release() problem for review.

Comment 25

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #24)
> Created attachment 8557170 [details] [diff] [review]
> Part 5 - Ensure the widget continues to be destroyed on the main thread
> 
> Posting Kats' fix for the Release() problem for review.

BenWa: The Part 1 and Part 2 patches in this bug provide useful context for this review. You can ignore the other patches.

Comment 26

[Benoit Girard (:BenWa)]

Actually are you sure that the object you destroy will still be alive by the time the Destroy message is processed?

Comment 27

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

Yeah the posted task keeps a refptr to the ChromeProcessController.

Comment 28

[Botond Ballo [:botond]]

(In reply to Benoit Girard (:BenWa) from comment #26)
> Actually are you sure that the object you destroy will still be alive by the
> time the Destroy message is processed?

It should be, because NewRunnableMethod calls AddRef() on the object when creating the task [1] (and Release() after it's executed).

[1] http://mxr.mozilla.org/mozilla-central/source/ipc/chromium/src/base/task.h?rev=69cdf42cdaf9#263

Comment 29

[Botond Ballo [:botond]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1b374d0963b9
https://hg.mozilla.org/integration/mozilla-inbound/rev/e000b9c887f9
https://hg.mozilla.org/integration/mozilla-inbound/rev/638841804bcb
https://hg.mozilla.org/integration/mozilla-inbound/rev/b026c62b1728
https://hg.mozilla.org/integration/mozilla-inbound/rev/3b36478f35fd

Comment 30

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/1b374d0963b9
https://hg.mozilla.org/mozilla-central/rev/e000b9c887f9
https://hg.mozilla.org/mozilla-central/rev/638841804bcb
https://hg.mozilla.org/mozilla-central/rev/b026c62b1728
https://hg.mozilla.org/mozilla-central/rev/3b36478f35fd

Comment 31

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

Comment on attachment 8553937 [details] [diff] [review]
Part 1 - Store the widget in ChromeProcessController

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): required for parent-process APZ (bug 950934) and input-thread (bug 930939) which are 2.2+ features. I'm tracking the full set of bugs that will need uplifting together at http://mzl.la/1zvKgmk; requesting approval on bugs individually but will wait until everything has approval before I uplift any of it.
User impact if declined: can't have parent-process APZ or input-thread
Testing completed: on master. some of these bugs have been baking for a while; others are more recent.
Risk to taking this patch (and alternatives if risky): there's likely some edge cases that we haven't run into yet and will be uncovered with further testing. Bug 1128887 is the only known issue that we don't yet have a fix for but I consider that relatively minor and something we can fix after uplifting everything else.
String or UUID changes made by this patch: none

Comment 32

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

remote:   https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/6acb96eb5eb2
remote:   https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/b3741a822957
remote:   https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/8132b81de165
remote:   https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/36aff8057a92
remote:   https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/1509dacf2ade