Closed Bug 1124452 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: Panning and Zooming, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla38
Tracking Status
b2g-v2.2 --- fixed
b2g-master --- fixed

People

(Reporter: botond, Assigned: botond)

References

Details

(Whiteboard: [input-thread-uplift-part1])

Attachments

(6 files, 2 obsolete files)

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.
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.
Depends on: containerless-root
Assignee: nobody → botond
(Originally posted as Part 2 patch in bug 1005815. Moving it here because I need the widget in the next patch.)
Attachment #8553937 - Flags: review?(bugmail.mozilla)
Timothy, I took a stab at how to get the document element from the widget. Let me know if this is reasonable.
Attachment #8553940 - Flags: review?(tnikkel)
Attachment #8553940 - Flags: review?(bugmail.mozilla)
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:
Attachment #8553937 - Flags: review?(bugmail.mozilla) → review+
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.
Attachment #8553940 - Flags: review?(bugmail.mozilla) → review+
Attachment #8553940 - Flags: review?(tnikkel) → review+
(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. :)
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.
Attachment #8553940 - Flags: review+ → review-
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
Attached patch Additional fixup (obsolete) — Splinter Review
Additional fixup I needed to do. Feel free to steal/combine into other patches.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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:kats@mozilla.com) 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
No longer depends on: containerless-root
(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...
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.
Attachment #8555995 - Flags: review+
Now including the fix for the infinite recursion.
Attachment #8553940 - Attachment is obsolete: true
Attachment #8555997 - Flags: review?(tnikkel)
Attachment #8555997 - Flags: review?(bugmail.mozilla)
This is the rest of Kats' "Additional fixup" patch (the fix for the infinite recursion part was moved into Part 3).
Attachment #8554590 - Attachment is obsolete: true
Attachment #8555999 - Flags: review?(tnikkel)
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.
Attachment #8555997 - Flags: review?(bugmail.mozilla) → review+
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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.
Attachment #8555997 - Flags: review?(tnikkel) → review+
Attachment #8555999 - Flags: review?(tnikkel) → review+
(In reply to Botond Ballo [:botond] from comment #18)
> Also building locally to test.

Manual testing didn't reveal any obvious breakage.
Looks like we're getting a crash in nsBaseWidget::Release() with these patches.
I think the crash is likely because the ChromeProcessController is getting destroyed on the compositor thread, and nsIWidget doesn't have threadsafe addref'ing.
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
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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.
Posting Kats' fix for the Release() problem for review.
Attachment #8557170 - Flags: review?(bgirard)
(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.
Attachment #8557170 - Flags: review?(bgirard) → review+
Actually are you sure that the object you destroy will still be alive by the time the Destroy message is processed?
Yeah the posted task keeps a refptr to the ChromeProcessController.
(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
Depends on: 1128527
Whiteboard: [NO_UPLIFT][input-thread-uplift-part1]
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
Attachment #8553937 - Flags: approval-mozilla-b2g37?
Attachment #8555995 - Flags: approval-mozilla-b2g37?
Attachment #8555997 - Flags: approval-mozilla-b2g37?
Attachment #8555999 - Flags: approval-mozilla-b2g37?
Attachment #8557170 - Flags: approval-mozilla-b2g37?
Attachment #8553937 - Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+
Attachment #8555995 - Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+
Attachment #8555997 - Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+
Attachment #8555999 - Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+
Attachment #8557170 - Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+
You need to log in before you can comment on or make changes to this bug.