Closed Bug 1675376 Opened 3 years ago Closed 3 years ago

Tab is unresponsive if dialog is closed while initial preview is rendering

Categories

(Core :: Print Preview, defect, P1)

defect

Tracking

()

VERIFIED FIXED
85 Branch
Tracking Status
firefox84 --- verified
firefox85 --- verified

People

(Reporter: mstriemer, Assigned: emilio)

References

Details

(Whiteboard: [print2020_v84][old-ui+])

Attachments

(4 files)

STR

  1. Go to http://www.worldslongestwebsite.com/
  2. Start a print
  3. Wait for the print settings pane to load (the preview should start rendering shortly after that)
  4. Hit Cancel/Escape before the initial preview finishes rendering

Expected results: The print dialog is closed and the page works as it did before the dialog opened
Actual results: The dialog is closed, but you can't select text or refresh the page. Switching away from the tab and back to it shows a spinner. Closing the tab and then reopening it with Ctrl+Shift+T brings back a different tab

Performing the same STR on https://html.spec.whatwg.org/ has the page unresponsive for some time after closing the dialog, but it appears to start working again after a bit.

This appears to be the same issue that I noticed when looking at the JSWindowActor refactor of the old UI. When the initial preview is cancelled then the tab would be unresponsive for a bit. If the document is large enough this could take several minutes to be resolved (on my Win 10 Ryzen 1600 PC).

Here's a profile [1] of cancelling the print while the initial preview is rendering for http://www.worldslongestwebsite.com/. I ended the profile at 45 seconds, since it takes a few minutes to resolve. There is a large chunk there were the reflow code is running.

I checked mozregression and it was pretty clearly going to resolve to the first patch with previews in the tab modal UI.

Jonathan, any idea what could be done here? It looks like the preview code keeps running for a while on the initial render even after we've closed the dialog.

[1] https://share.firefox.dev/3km2NAF

Flags: needinfo?(jwatt)
Whiteboard: [print2020_v84] → [print2020_v84][old-ui+]

Moving to Core :: Print Preview since this seems like it is stuck in platform code.

Component: Printing → Print Preview
Product: Toolkit → Core

Emilio: Could you take a quick look?

Flags: needinfo?(emilio)

Yeah, so this is an O(n^2) dependent on the number of pages. GetLogicalSkipSides calls into GetEffectiveComputedBSize, which computes the ConsumedBSize by looping through all the previous siblings... which is obviously bad.

We should be able to cache the consumed bsize somehow.

Assignee: nobody → emilio

Maybe we could cache a frame's ConsumedBSize in the frame's previous continuation (as a frame property) when it's access the first time so that we don't need to traverse all its previous continuations every time ConsumedBSize is called.

Depends on: 1677816

The caller has it, so let's pass it down so that we don't need to compute it on
demand.

This removes virtually all the time under ConsumedBSize. See the comment for
what ensures the correctness of the cache: Basically, we refresh the cache for
a frame continuation every time we reflow it, which means that when next
continuations go look for it it should be up-to-date (we rely on that already
because we're looking at the content rect).

Depends on D97356

That avoids going all the way to the first continuation to call ResolveBidi on
it. This shaves a bunch of time when there are a lot of pages.

This is more problematic than it seems specially when there's no bidi, because
in the "bidi not enabled" case we never remove the flag, which is bad. When
bidi is actually enabled we usually have done the resolution already.

Depends on D97357

https://treeherder.mozilla.org/jobs?repo=try&revision=12710df9f8a94b0196f6ec11cc7d48fb3c356367

I think this should do for now. I don't see any obvious cliffs after that in the profiler.

Flags: needinfo?(emilio)
Flags: needinfo?(jwatt)
Blocks: 1677917
Blocks: 1677920
Pushed by ealvarez@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e3aa6fb62422
Pass consumed bsize to ApplySkipSides when during reflow. r=mats
https://hg.mozilla.org/integration/autoland/rev/20d5c7217304
Cache consumed BSize in a frame property for non-first continuations. r=mats
https://hg.mozilla.org/integration/autoland/rev/b276c591ea2d
Lift IsBidiEnabled check up to the caller. r=mats

That test is very sensitive to stack size, and asan is somehow increasing that after the first patch. I'll skip them for asan here and un-skip them in bug 1677917.

Flags: needinfo?(emilio)
Pushed by ealvarez@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6cde83326498
Pass consumed bsize to ApplySkipSides when during reflow. r=mats
https://hg.mozilla.org/integration/autoland/rev/54da9db54cbe
Cache consumed BSize in a frame property for non-first continuations. r=mats
https://hg.mozilla.org/integration/autoland/rev/3abee7381c9a
Lift IsBidiEnabled check up to the caller. r=mats

Just a reminder to request uplift on this one. I think we'll want this for the 84 experiment.

Flags: needinfo?(emilio)

Comment on attachment 9188406 [details]
Bug 1675376 - Pass consumed bsize to ApplySkipSides when during reflow. r=mats,TYLin

Beta/Release Uplift Approval Request

  • User impact if declined: Slow printing in sites with many pages.
  • Is this code covered by automated tests?: Yes
  • Has the fix been verified in Nightly?: No
  • Needs manual test from QE?: Yes
  • If yes, steps to reproduce: Open https://html.spec.whatwg.org, print, compare speed.
  • List of other uplifts needed: none
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): Tried very hard to make it ~impossible to change actual behavior, should just be faster.
  • String changes made/needed: none
Flags: needinfo?(emilio)
Attachment #9188406 - Flags: approval-mozilla-beta?
Attachment #9188407 - Flags: approval-mozilla-beta?
Attachment #9188408 - Flags: approval-mozilla-beta?
Flags: qe-verify+
QA Whiteboard: [qa-triaged]

Comment on attachment 9188406 [details]
Bug 1675376 - Pass consumed bsize to ApplySkipSides when during reflow. r=mats,TYLin

Approved for 84.0b3.

Attachment #9188406 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #9188407 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #9188408 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attached image TabUnresponsive.gif

Hi Emilio!

Unfortunately, I'm still encountering the tab unresponsiveness.

It seems that, hitting CTRL + P still doesn't open the print preview, buttons like (refresh or homepage) doesn't seem to work and trying to navigate away by typing something else inside the Address bar is not possible (until waiting for a long period of time) .

This happens on both http://www.worldslongestwebsite.com/ & https://html.spec.whatwg.org/ but these symptoms are worst for the first link.

Also, I have encountered Bug 1679023 after stressing the affected tab with Fission enabled

Flags: needinfo?(emilio)

Yeah, so the behavior in html.spec.whatwg.org is roughly as expected. Interaction takes longer because we're busy reflowing, but it completes relatively soon.

The behavior in http://www.worldslongestwebsite.com/ is still relatively bad, so I suspect there's some other bug to hunt down there. Will move that to bug 1679023.

Flags: needinfo?(emilio)
Blocks: 1679023

Thanks Emilio!

Marking this as verified fixed for Firefox 85.0a1 (BuildId:20201124092228) and Firefox 84.0b4 (BuildId:20201122152513) based on the html.spec.whatwg.org improved behavior.

Status: RESOLVED → VERIFIED
Flags: qe-verify+
Regressions: 1686849
Depends on: 1715098
No longer depends on: 1715098
Regressions: 1715098
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: