Tab is unresponsive if dialog is closed while initial preview is rendering
Categories
(Core :: Print Preview, defect, P1)
Tracking
()
People
(Reporter: mstriemer, Assigned: emilio)
References
Details
(Whiteboard: [print2020_v84][old-ui+])
Attachments
(4 files)
47 bytes,
text/x-phabricator-request
|
RyanVM
:
approval-mozilla-beta+
|
Details | Review |
47 bytes,
text/x-phabricator-request
|
RyanVM
:
approval-mozilla-beta+
|
Details | Review |
47 bytes,
text/x-phabricator-request
|
RyanVM
:
approval-mozilla-beta+
|
Details | Review |
2.25 MB,
image/gif
|
Details |
STR
- Go to http://www.worldslongestwebsite.com/
- Start a print
- Wait for the print settings pane to load (the preview should start rendering shortly after that)
- 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.
Reporter | ||
Comment 1•3 years ago
|
||
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.
Reporter | ||
Comment 2•3 years ago
|
||
Moving to Core :: Print Preview since this seems like it is stuck in platform code.
Assignee | ||
Comment 4•3 years ago
|
||
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 | ||
Comment 5•3 years ago
|
||
Assignee | ||
Updated•3 years ago
|
Comment 6•3 years ago
|
||
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.
Assignee | ||
Comment 7•3 years ago
|
||
The caller has it, so let's pass it down so that we don't need to compute it on
demand.
Assignee | ||
Comment 8•3 years ago
|
||
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
Assignee | ||
Comment 9•3 years ago
|
||
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
Assignee | ||
Comment 10•3 years ago
|
||
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.
Assignee | ||
Updated•3 years ago
|
Comment 11•3 years ago
|
||
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
Comment 12•3 years ago
|
||
Backed out 3 changesets (bug 1675376) for asan crashtest failures.
Backout link: https://hg.mozilla.org/integration/autoland/rev/391bda1947b17fe694294ae363ede252c40066e7
Failure log: https://treeherder.mozilla.org/logviewer?job_id=322144330&repo=autoland&lineNumber=17948
Assignee | ||
Comment 13•3 years ago
|
||
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.
Comment 14•3 years ago
|
||
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
Comment 15•3 years ago
|
||
Just a reminder to request uplift on this one. I think we'll want this for the 84 experiment.
Comment 16•3 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/6cde833264983df1d8bb92f4051cf8ece18ec221
https://hg.mozilla.org/mozilla-central/rev/54da9db54cbef62e84c722dba6d65943af8d9e58
https://hg.mozilla.org/mozilla-central/rev/3abee7381c9a35ede4693e3d7c8c3264d7858ca5
Assignee | ||
Comment 17•3 years ago
|
||
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
Assignee | ||
Updated•3 years ago
|
Updated•3 years ago
|
Comment 18•3 years ago
|
||
Comment on attachment 9188406 [details]
Bug 1675376 - Pass consumed bsize to ApplySkipSides when during reflow. r=mats,TYLin
Approved for 84.0b3.
Updated•3 years ago
|
Updated•3 years ago
|
Comment 19•3 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/ef5e6a1e74de
https://hg.mozilla.org/releases/mozilla-beta/rev/c0132838507d
https://hg.mozilla.org/releases/mozilla-beta/rev/2ffa4440e59e
Comment 20•3 years ago
|
||
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
Assignee | ||
Comment 21•3 years ago
|
||
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.
Comment 22•3 years ago
|
||
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.
Assignee | ||
Updated•3 years ago
|
Description
•