Botond Ballo [:botond] Assignee
	Description • 9 years ago

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.

	Botond Ballo [:botond] Assignee
	Comment 1 • 9 years ago

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.

	Botond Ballo [:botond] Assignee
	Comment 2 • 9 years ago

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

	Botond Ballo [:botond] Assignee
	Comment 3 • 9 years ago

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

	Botond Ballo [:botond] Assignee
	Comment 4 • 9 years ago

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

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 5 • 9 years ago

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:

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 6 • 9 years ago

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.

	Timothy Nikkel (:tnikkel)
	Comment 7 • 9 years ago

(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. :)

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 8 • 9 years ago

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.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 9 • 9 years ago

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

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 10 • 9 years ago

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

	Botond Ballo [:botond] Assignee
	Comment 11 • 9 years ago

(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

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 12 • 9 years ago

(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...

	Botond Ballo [:botond] Assignee
	Comment 13 • 9 years ago

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.

	Botond Ballo [:botond] Assignee
	Comment 14 • 9 years ago

Now including the fix for the infinite recursion.

	Botond Ballo [:botond] Assignee
	Comment 15 • 9 years ago

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

	Botond Ballo [:botond] Assignee
	Comment 16 • 9 years ago

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

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 17 • 9 years ago

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.

	Botond Ballo [:botond] Assignee
	Comment 18 • 9 years ago

(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.

	Botond Ballo [:botond] Assignee
	Comment 19 • 9 years ago

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

Manual testing didn't reveal any obvious breakage.

	Botond Ballo [:botond] Assignee
	Comment 20 • 9 years ago

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

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 21 • 9 years ago

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

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 22 • 9 years ago
try

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

	Botond Ballo [:botond] Assignee
	Comment 23 • 9 years ago

(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.

	Botond Ballo [:botond] Assignee
	Comment 24 • 9 years ago

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

	Botond Ballo [:botond] Assignee
	Comment 25 • 9 years ago

(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.

	Benoit Girard (:BenWa)
	Comment 26 • 9 years ago

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

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 27 • 9 years ago

Yeah the posted task keeps a refptr to the ChromeProcessController.

	Botond Ballo [:botond] Assignee
	Comment 28 • 9 years ago

(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

	Botond Ballo [:botond] Assignee
	Comment 29 • 9 years ago
landing

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

	Phil Ringnalda (:philor)
	Comment 30 • 9 years ago

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

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 31 • 9 years ago

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

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 32 • 9 years ago

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