Closed Bug 1511381 Opened 6 years ago Closed 6 years ago

Synchronous layout flush coming from PluginChild.jsm during page load

Categories

(Core Graveyard :: Plug-ins, enhancement, P1)

Product:
Core Graveyard
Component:
Plug-ins
Type:
enhancement

Tracking

(Performance Impact:high, firefox67 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla67
Project Flags:
Performance Impact high
Tracking Flags:
Tracking Status
firefox67 --- fixed

People

(Reporter: smaug, Assigned: timdream)

References

(
URL
)

Details

(Keywords: perf:pageload)

Attachments

(1 file, 3 obsolete files)

Bug 1511381 - Prevent forced layout flush when click-to-play UI loads on page load
47 bytes, text/x-phabricator-request
Details | Review
http://bit.ly/2RnVlY5 This might be related to UA Widgets and those changing the timing when widgets are initialized. There is UAWidgetsChild.jsm on stack when PluginChild.jsm code is executed.
This is probably a [qf:p1:pageload] bug.
This seems to be reproducedable with cnn.com
URL: https://edition.cnn.com/
With click-to-play turned on right? I don't remember our default plugin policy right now... This is probably the offender. https://searchfox.org/mozilla-central/rev/8f0db72fb6e35414fb9a6fc88af19c69f332425f/browser/actors/PluginChild.jsm#269
Actually, the first offender is |overlay.scrollWidth| https://searchfox.org/mozilla-central/rev/8f0db72fb6e35414fb9a6fc88af19c69f332425f/browser/actors/PluginChild.jsm#262 I wonder if I could tweak the script to workaround this without having to spend too much time rewriting it...
Assignee: nobody → timdream
Status: NEW → ASSIGNED
Attached patch patch (obsolete) — — Splinter Review
For some reason, I cannot get this uploaded to Phabricator...
Attachment #9029089 - Flags: review?(felipc)
This is similar to bug 1493525, in which an UA Widget constructor causes an early, visible force reflow. The fix here makes the offending method (computeAndAdjustOverlayDisplay()) async and have it wait for an rAF callback. The order of the code execution is audited and changed, so that tapping into the overlay and the plugin elements after the await should not cause any problems, including if the plugin is already removed (there is already a test for that.) A few tests need some tweaking accordingly to make sure it asserts the overlay after it is decorated with the right state according to its size.
Attachment #9029089 - Attachment is obsolete: true
Attachment #9029089 - Flags: review?(felipc)
There are still tests that need to be adjusted; for some reason, they don't show up locally on my macOS. https://treeherder.mozilla.org/#/jobs?repo=try&selectedJob=214994424&revision=cb924f5c8a758a6fc1c6159f8b6eaec8df33b1e0
This is similar to bug 1493525, in which an UA Widget constructor causes an early, visible force reflow. The fix here makes the offending method (computeAndAdjustOverlayDisplay()) async and have it wait for an rAF callback. The order of the code execution is audited and changed, so that tapping into the overlay and the plugin elements after the await should not cause any problems, including if the plugin is already removed (there is already a test for that.) A few tests need some tweaking accordingly to make sure it asserts the overlay after it is decorated with the right state according to its size.
Attachment #9029126 - Attachment is obsolete: true
Whiteboard: [qf] → [qf:p1:pageload]
Attachment #9029430 - Attachment description: Bug 1511381 - Measure the size of plugin overlay only after an rAF callback → Bug 1511381 - Measure the size of plugin overlay after promiseDocumentFlushed
Attachment #9029430 - Attachment is obsolete: true

This bug might appear as stalled here but there was a discussion here.

https://phabricator.services.mozilla.com/D13731#inline-80422

I am exploring other routes to see if it's possible to find a right timing for UA Widget.

So I've spent some time figuring out what I can do to make this work.

I looked at two fronts: (1) if we could get a tick after layout flush, (2) see if we can forgo the requirement of reading dimensions from the layout.

For (1) I tried to (a) abuse the overflow & underflow event — I did that by always set the full UI when there isn't a layout (which will overflow), and only try to resize the UI again upon hearing the overflow event.

(a) helped me a lot in terms of deciding the "sizing" and the "notext" attributes; i.e. the visibility if the icons and texts (Part I.)

However, for other test cases, particularly around the feature that detects other elements overlaying the plug-in (Part II), we do need a "layout flushed" callback in order to make a correct measurement. So far I've failed to find one for that -- suggestions in the thread on the previous patch are not available, neither does (b) ReflowObserver. I am not sure if I am using the reflow observer correctly, though.

For (2), I investigated at the commit history. bug 1405655 comment 17 is very useful. I don't think we will be able to implement hit area detection ("Part II") otherwise though.

Part I is essentially trying to implement CSS element query/container query. We could get rid of that by actually "upgrade" the UI to an iframe and use media query (lol) but that feels like an overkill.

Perhaps I should double down (a) by setting the content to be really really wide when the size is indeterminate -- that way I will always get a callback for Part II, not just Part I.

I will upload my currently failing patch here.

This patch attempts to probe into the layout state and set the overlay attribute accordingly. It kind of works except for the part where we really do need the correct dimensions and coornidates for the elementFromPoint() tests.

I've just updated the patch; these profiles confirm the fix is valid.

I tested this on https://get.adobe.com/flashplayer/about/ which comes with two CTP UIs.

After the patch, no call into PresShell::DoFlushPendingNotifications from computeAndAdjustOverlayDisplay: http://bit.ly/2Rs48HQ

Before the patch: http://bit.ly/2RnHuAs

I am not sure which tests will break with the patch. All the test scripts wait for the page to load so we shouldn't need changes on every test:

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

Attachment #9034929 - Attachment description: Bug 1511381 - WIP Prevent forced layout flush when click-to-play UI loads → Bug 1511381 - Prevent forced layout flush when click-to-play UI loads on page load
Pushed by timdream@gmail.com: https://hg.mozilla.org/integration/autoland/rev/3e8eaa5b7937 Prevent forced layout flush when click-to-play UI loads on page load r=mconley

https://hg.mozilla.org/mozilla-central/rev/3e8eaa5b7937

Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox67: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla67
Performance Impact: --- → P1
Keywords: perf:pageload
Whiteboard: [qf:p1:pageload]
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: