Closed
Bug 881832
Opened 11 years ago
Closed 9 years ago
Make inner document reflow asynchronous to improve performance
Categories
(Core :: Layout, defect)
Core
Layout
Tracking
()
People
(Reporter: MatsPalmgren_bugz, Assigned: mattwoodrow)
References
Details
(Keywords: perf)
Attachments
(11 files, 14 obsolete files)
(deleted),
text/plain
|
Details | |
(deleted),
text/plain
|
Details | |
(deleted),
text/plain
|
Details | |
(deleted),
text/plain
|
Details | |
(deleted),
patch
|
dbaron
:
review+
lizzard
:
approval-mozilla-esr45+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
mattwoodrow
:
review+
lizzard
:
approval-mozilla-esr45+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
dbaron
:
review+
lizzard
:
approval-mozilla-esr45+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
dbaron
:
review+
lizzard
:
approval-mozilla-esr45+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
tnikkel
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
mattwoodrow
:
feedback+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
tnikkel
:
review+
|
Details | Diff | Splinter Review |
There's a nsSubDocumentFrame::ReflowFinished callback that leads to
nsViewManager::DoSetWindowDimensions which calls PresShell::ResizeReflow.
So reflowing the inner document is basically synchronous.
(see attached stack)
Reporter | ||
Comment 1•11 years ago
|
||
This patch brakes a few tests though:
https://tbpl.mozilla.org/?tree=Try&rev=16da08a6f203
content/html/document/test/test_bug741266.html
M-1: "Popup height should be 100 when opened with window.open - got 822, expected 100"
chrome/docshell/test/chrome/test_bug113934.xul
M-oth: " Should reflow on swap - Expected ..."
browser/devtools/responsivedesign/test/browser_responsiveui.js
M-bc: "preset 7: dimension valid (width) - Got 1160, expected 1920"
Attachment #761071 -
Flags: review?(tnikkel)
Reporter | ||
Comment 2•11 years ago
|
||
This fixes two test by adding setTimeout(..., 0) in a couple of places.
content/html/document/test/test_bug741266.html
docshell/test/chrome/bug113934_window.xul
I need help fixing this test though:
browser/devtools/responsivedesign/test/browser_responsiveui.js
Comment 3•11 years ago
|
||
I don't know if setTimeout in test_bug741266.html is the right way to fix that. We might flush the subdocument after the setTimeout runs, I don't think there is anything to guarantee a flush will happen before the setTimeout runs.
innerWidth/Height should return correct values. Maybe we are running into bug 641188? I have a patch for that that is green, but it is sort of blocked on having a decent solution for bug 871434.
Reporter | ||
Comment 4•11 years ago
|
||
Comment on attachment 761075 [details] [diff] [review]
fix tests, except one
OK, "it should just work" works for me ;-)
Attachment #761075 -
Attachment is obsolete: true
Reporter | ||
Comment 5•11 years ago
|
||
Is the patch in bug 641188 the latest you have? (it seems to have bitrotted)
I'd like to test it with the patch in this bug to see if it's green with it.
Comment 6•11 years ago
|
||
I just uploaded my latest, although they might still need to be refreshed, it should be easy. "flush delayed resize" is the one that I think should be green. "look at the docshell size or prescontext visible area depending on if the visible area is overridden" exposes bug 871434.
Reporter | ||
Comment 7•11 years ago
|
||
Thanks! All three tests pass unmodified with that patch in my tree. Nice!
https://tbpl.mozilla.org/?tree=Try&rev=ec4e8271d5c8
Comment 8•11 years ago
|
||
Comment on attachment 761071 [details] [diff] [review]
fix
UUID bumps in two idl files please.
> nsDocShell::SetPositionAndSize(int32_t x, int32_t y, int32_t cx,
>- int32_t cy, bool fRepaint)
>+ int32_t cy, uint32_t aFlags)
> nsCOMPtr<nsIContentViewer> viewer = mContentViewer;
> if (viewer) {
> //XXX Border figured in here or is that handled elsewhere?
>- NS_ENSURE_SUCCESS(viewer->SetBounds(mBounds), NS_ERROR_FAILURE);
>+ NS_ENSURE_SUCCESS(viewer->SetBoundsWithFlags(mBounds, aFlags), NS_ERROR_FAILURE);
Do we want to use nsIBaseWindow flags on nsIContentViewer? Usually we would define flags on nsIContentViewer to use. Since nsIContentViewer ignores the repaint flag I think that would be a good idea.
Reporter | ||
Comment 9•11 years ago
|
||
So with bug 641188 "flush delayed resize" there are these test failures:
M-oth: layout/style/test/chrome/test_hover.html
"Test timed out."
M-oth: content/events/test/test_bug602962.xul
"Scroll X should not have changed yet - got 0, expected 200"
M-oth: toolkit/content/tests/chrome/test_bug437844.xul
":hover applies - got transparent, expected rgb(0, 255, 0)"
M-bc: browser/components/tabview/test/browser_tabview_bug625269.js
"Much smaller: The group should have been resized - Got false, expected true"
Comment 10•11 years ago
|
||
That is surprising. FWIW the patch from bug 641188 is green https://tbpl.mozilla.org/?tree=Try&rev=0ddb5fcf8458
Updated•11 years ago
|
Blocks: CVE-2016-5283
Comment 11•11 years ago
|
||
Comment on attachment 761071 [details] [diff] [review]
fix
Other than comment 8 and test fixes this patch looks good. I'll just grant r+ to get this off my queue.
Attachment #761071 -
Flags: review?(tnikkel) → review+
Reporter | ||
Comment 13•11 years ago
|
||
Bumped uuids and fixed nits.
Attachment #761071 -
Attachment is obsolete: true
Attachment #8345942 -
Flags: review+
Reporter | ||
Comment 14•11 years ago
|
||
I tried to fix test_hover.html but failed. It depends on hover, style
changes, and scroll/resize events happening in specific order which
I think is impossible to maintain with this patch. We need to rewrite
this test in some other way.
Looks green so far, I'll do a Try run for Android as well...
https://tbpl.mozilla.org/?tree=Try&rev=bce546876e66
Reporter | ||
Comment 15•11 years ago
|
||
Comment on attachment 8345945 [details] [diff] [review]
Fix test_bug602962.xul and disable test_hover.html
Green also on Android. https://tbpl.mozilla.org/?tree=Try&rev=29f522bd791a
I think we should land this and rewrite test_hover.html at a later time.
Timothy, do you agree?
Attachment #8345945 -
Flags: feedback?(tnikkel)
Comment 16•11 years ago
|
||
Comment on attachment 8345945 [details] [diff] [review]
Fix test_bug602962.xul and disable test_hover.html
Just had a quick look at test_hover. Do we know why it is failing exactly? It looks like we check iframe.contentDocument.body.offsetWidth anytime we expect it to change size. Shouldn't that flush out the document resize?
Comment 17•11 years ago
|
||
Comment on attachment 8345945 [details] [diff] [review]
Fix test_bug602962.xul and disable test_hover.html
I think I'd like an explanation to comment 16 before +'ing this.
Attachment #8345945 -
Flags: feedback?(tnikkel) → feedback-
Reporter | ||
Comment 18•11 years ago
|
||
Perhaps it's better if someone else takes a fresh look at the remaining issue...
Assignee: matspal → nobody
Comment 20•11 years ago
|
||
Is there an actual failure here, or just a timeout? :tn made this a chrome: test in 593262 to fix some other timeout. Does it still need to be a chrome: test now that we're doing asynch reflow? Is it green if we revert 593262?
Flags: needinfo?(tnikkel)
Flags: needinfo?(matspal)
Updated•11 years ago
|
Flags: needinfo?(bugs)
Comment 21•11 years ago
|
||
I rebased and pushed to try to see where we stand now
https://tbpl.mozilla.org/?tree=Try&rev=fe38461fd169
Comment 22•11 years ago
|
||
regular mochitest test_hover with patch
https://tbpl.mozilla.org/?tree=Try&rev=3d4b757d397c
regular mochitest test_hover without patch
https://tbpl.mozilla.org/?tree=Try&rev=5e431fe10d85
I tried making test_hover a regular mochitest, it still fails (but passes without the patch). And it looks like we now failing browser/components/tabview/test/browser_tabview_bug625269.js too. On the plus side we don't seem to be failing test_bug602962.xul anymore (at least on Linux).
Reporter | ||
Comment 23•11 years ago
|
||
(In reply to Jet Villegas (:jet) from comment #20)
> Is there an actual failure here, or just a timeout?
It might be an actual failure, but we need to understand the exact nature
of the test failures to be able to tell. I was looking at test_hover.html
which is rather complicated, so browser_tabview_bug625269.js is probably
a better starting point.
Flags: needinfo?(matspal)
Comment 24•11 years ago
|
||
Mats, do you think it's possible that we are getting into a situation like the one described here? http://mxr.mozilla.org/mozilla-central/source/view/src/nsViewManager.cpp#212
If we are asked to delay are resize in that function, and we overwrite a value in mDelayedResize it seems possible we get the same problem.
Flags: needinfo?(matspal)
Comment 25•11 years ago
|
||
Just stashing a copy of this here to avoid having to unbitrot again.
Reporter | ||
Comment 26•11 years ago
|
||
I tried that, doing a FlushDelayedResize(false) in the else-part too
under similar conditions. It didn't help, test_hover.html still failed.
Flags: needinfo?(matspal)
Comment 27•11 years ago
|
||
(Clearing needinfo. Unlikely to have time to dig deeper into this in the short term.)
Updated•11 years ago
|
Flags: needinfo?(tnikkel)
Updated•11 years ago
|
Flags: needinfo?(tnikkel)
Flags: needinfo?(matspal)
Comment 28•11 years ago
|
||
Is it possible to change the tests so that they still test what they were originally intended to test, but with appropriate waiting as required by the changes here?
Reporter | ||
Comment 29•11 years ago
|
||
I debugged the code and testcase (test_hover.html) again and I think it might have
something to do with this: http://hg.mozilla.org/mozilla-central/rev/7b553bbed53d
IOW, the problem might just be with 'synthesizeMouse' used in mochitests and not
in code we ship?
I tried to make the test pass, by inserting style changes that would cause reflow
or frame construction in various places, changing timeouts etc. The test still fails
or hangs.
Flags: needinfo?(matspal)
Reporter | ||
Comment 30•11 years ago
|
||
Attachment #8384894 -
Attachment is obsolete: true
Comment 31•10 years ago
|
||
Can we just disable the test and land this if nobody has time to fix the test?
Comment 32•10 years ago
|
||
David: do you agree with Mats' findings? ie. that our synthetic mouse hover is faulty?
Flags: needinfo?(dbaron)
Comment 33•10 years ago
|
||
I'm pretty hesitant to disable test_hover.html since it covers functionality that has relatively poor test coverage and has regressed quite a few times over the years.
I believe at least some of the tests in test_hover.html were designed explicitly to test the behavior fixed in http://hg.mozilla.org/mozilla-central/rev/7b553bbed53d so that it wouldn't break a third time.
Is there a newer version of the patch to test than the one in comment 31?
Flags: needinfo?(dbaron)
Reporter | ||
Comment 34•10 years ago
|
||
Updated for recent directory changes. This applies cleanly to latest
m-i (rev 0f6aedc8d6e4).
Attachment #8418975 -
Attachment is obsolete: true
Updated•10 years ago
|
Flags: needinfo?(dbaron)
Comment 35•10 years ago
|
||
Any update here? This bug is about a year and a half old now.
Reporter | ||
Comment 36•10 years ago
|
||
Applies to m-c 35df417b93a7.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2a48aa9f968b
Attachment #8509706 -
Attachment is obsolete: true
Reporter | ||
Comment 37•10 years ago
|
||
The latest test run has a few failures in layout/base/tests/test_reftests_with_caret.html
which appears to be new. It looks like some element in the test is unfocused when it
should have been focused. Other than that, it's the same failures as earlier.
Reporter | ||
Comment 38•10 years ago
|
||
based on m-c rev 1dd013ece082.
Pushed to Try to re-check test_reftests_with_caret test:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=42d8dd6eb2a4
Attachment #8550816 -
Attachment is obsolete: true
Reporter | ||
Comment 39•10 years ago
|
||
Reporter | ||
Comment 40•10 years ago
|
||
Compared with the previous log there is a lot more oscillation with
synthetic mouse events now. I tried to avoid that in various ways
but couldn't find something that works.
Reporter | ||
Comment 41•10 years ago
|
||
So here's a stack to nsViewManager::SetWindowDimensions that comes from
a flush when doing a PresShell::DispatchSynthMouseMove (it results
in a delayed resize), and the subsequent DoSetWindowDimensions when
that resize is executed. It *might* show how the oscillation is driven.
Reporter | ||
Updated•10 years ago
|
Attachment #8555915 -
Attachment is patch: false
Reporter | ||
Updated•10 years ago
|
Attachment #8555396 -
Attachment is patch: true
Reporter | ||
Comment 42•10 years ago
|
||
Reporter | ||
Comment 43•10 years ago
|
||
Updated the patch to see if Olli's patch in bug 1149555
might help with the remaining test failure. It didn't.
Attachment #8555396 -
Attachment is obsolete: true
Comment 44•9 years ago
|
||
I did a little debugging of what's happening in test_hover.html here.
For a start, the "called only once" tests in the test were all broken by the test changes in https://hg.mozilla.org/mozilla-central/rev/4b879b793eb6 , which made the test only call all the resize handlers once whether or not the resize happened more than once.
Looking in the debugger, it appears that with your patch there is, in fact, a hover oscillation happening. I suspect it's not happening without the patch, but I haven't yet tested this. So I think that what happens is that the resize step moving from step5 to step6, or from step6 to step7, fails randomly depending on which half of the oscillation the code happens to be in before the change, and whether that triggers a resize. Or something like that.
A slightly bigger change we might want to make is to tie :hover updating to the refresh driver (and maybe also to layout flushes), as a different mechanism for ensuring we don't go into oscillations. (Doing it well would probably require more restyling, though, since the "right" way to do it would probably to be beginning every refresh cycle with nothing in :hover, updating layout, and then updating the :hover style based on that layout, perhaps even twice. Hopefully many of those changes could be optimized away given the lack of style changes, at least. But there might be a better way...)
Comment 45•9 years ago
|
||
There are oscillations without the patch; they're different, though.
I think the best path forward is probably to mark some tests as todo() rather than ok(), make sure that other failing-by-oscillation tests have the same early return that step4 currently does, and possibly to add some additional mouse moves to get out of the oscillating state before making the next change.
Comment 46•9 years ago
|
||
... oh, and along with that, I should have added, to fix the test to test what it was supposed to test in the first place, as I did (I think) in:
https://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/39693899a27c/test-hover-oscillate
Comment 47•9 years ago
|
||
Is that enough for you, or should I debug the test more closely?
For what it's worth, I have a merged version of the patch at:
https://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/37d4f9920476/mats-881832
in which I fixed two additional SetPositionAndSize calls. (You can see its revision history in that repository.)
Flags: needinfo?(mats)
Reporter | ||
Comment 48•9 years ago
|
||
Attachment #8345942 -
Attachment is obsolete: true
Attachment #8591363 -
Attachment is obsolete: true
Reporter | ||
Comment 49•9 years ago
|
||
(In reply to David Baron [:dbaron] ⌚UTC-8 from comment #49)
> Is that enough for you, or should I debug the test more closely?
I can't seem to make this test work. It hangs after step5 or step6.
(This is on top of your fix in comment 22.)
Flags: needinfo?(mats)
Reporter | ||
Comment 50•9 years ago
|
||
There are also additional failures to sort out on Windows:
layout/base/tests/test_reftests_with_caret.html
devtools/client/tilt/test/browser_tilt_zoom.js
Reporter | ||
Comment 51•9 years ago
|
||
> (This is on top of your fix in comment 22.)
Sorry, I meant on top of your fix in comment 48.
Mats are you still working on this?
Tracking for 46+ since other bugs appear to depend on this, and it still affects current versions.
status-firefox45:
--- → affected
status-firefox46:
--- → affected
status-firefox47:
--- → affected
tracking-firefox45:
--- → ?
tracking-firefox46:
--- → +
tracking-firefox47:
--- → +
Comment 53•9 years ago
|
||
Not tracking, too late for 45.
This looks amazing but also huge and complicated.
We're coming into the last week of aurora, so I'm wontfixing this for 46. Let's aim for 47.
But, if you land this and it looks great in 47 and you feel strongly about it, just let me know, or request uplift.
Assignee | ||
Comment 55•9 years ago
|
||
David, how strongly do you feel about modifying the test to work around the changes this patch makes?
I had a go, but there's a lot of oscillating, so we have to remove many of the 'called once' checks. We also end up in an unknown state for the next test, so we need code to handle both options.
I think we could instead keep the old behaviour (or fairly close to) without too much hassle.
The main regression from this patch is that our attempts to block synthetic mouse events from trigger more has been broken.
Our code at [1] attempts to flush reflow while the caller is still holding onto the current synthetic mouse event (so that no more can be requested).
With this patch the code runs for the parent document, and the DispatchEvent call sets up a delayed resize for the child document's (iframe) view.
Calling FlushPendingNotifications only flushes the parent document, and we don't reflow the child until later on when something else flushes it.
I tried a hacky fix using EnumerateSubDocuments to flush all descendants instead of only the current document, and that fixes the majority of the test problems.
Is there a 'right' way to ensure that we process the delayed resize for the child document from this code?
The remaining problem is that the test still expects two changes for some tests, one for the initial hover, and the one for the synthesized mouse event in response to the reflow (and all further synthesized mouse events to be blocked).
By deferring the resize until flush, we end up coalescing these two changes and decide that nothing has actually changed, so we don't fire a resize event.
We could fix this by detecting when we try defer a second size change before the first one has been flushed, and forcibly flush the first one at that point. Alternatively we could enjoy this optimization, and just change the test to stop expecting resize events to be fired.
I also noticed that layout/reftests/bugs/1242172-1.html fails now (it's a new test).
This happens when our first call to nsViewManager::FlushDelayedResize has aReflow=false, which updates the PresContext size, but doesn't reflow [2]. When we try actually flush this change, and call into PresShell::ResizeReflow(IgnoreOverride), our check for isHeightChanging [3] will always be false. I'm not sure how important this optimization is, we could maybe just drop it, or detect it another way.
[1] http://mxr.mozilla.org/mozilla-central/source/layout/base/nsPresShell.cpp#3659
[2] http://mxr.mozilla.org/mozilla-central/source/view/nsViewManager.cpp#233
[3] http://mxr.mozilla.org/mozilla-central/source/layout/base/nsPresShell.cpp#1838
Comment 56•9 years ago
|
||
(In reply to Matt Woodrow (:mattwoodrow) from comment #57)
> David, how strongly do you feel about modifying the test to work around the
> changes this patch makes?
Not particularly strongly, I think.
It sounds like you've got this problem in your head now -- and I don't, and it would probably take me a few hours to remember what's going on here -- so if you think you have a good path forwards you should probably just take it.
(At some point we should probably figure out if we want to switch to a Chromium-like model for mouseover and :hover where they're only updated once per refresh cycle, possibly with a separate high-frequency-mouseover event for those who need it.)
> I had a go, but there's a lot of oscillating, so we have to remove many of
> the 'called once' checks. We also end up in an unknown state for the next
> test, so we need code to handle both options.
>
> I think we could instead keep the old behaviour (or fairly close to) without
> too much hassle.
Keep the old behavior of the code or the test?
> The main regression from this patch is that our attempts to block synthetic
> mouse events from trigger more has been broken.
Hmmm. I thought I'd concluded that those attempts had already been broken by other things being async, but maybe I'm misremembering.
> Our code at [1] attempts to flush reflow while the caller is still holding
> onto the current synthetic mouse event (so that no more can be requested).
>
> With this patch the code runs for the parent document, and the DispatchEvent
> call sets up a delayed resize for the child document's (iframe) view.
>
> Calling FlushPendingNotifications only flushes the parent document, and we
> don't reflow the child until later on when something else flushes it.
>
> I tried a hacky fix using EnumerateSubDocuments to flush all descendants
> instead of only the current document, and that fixes the majority of the
> test problems.
>
> Is there a 'right' way to ensure that we process the delayed resize for the
> child document from this code?
Does flushing not do that?
Assignee | ||
Comment 57•9 years ago
|
||
(In reply to David Baron [:dbaron] ⌚️UTC-7 from comment #58)
>
> It sounds like you've got this problem in your head now -- and I don't, and
> it would probably take me a few hours to remember what's going on here -- so
> if you think you have a good path forwards you should probably just take it.
>
> (At some point we should probably figure out if we want to switch to a
> Chromium-like model for mouseover and :hover where they're only updated once
> per refresh cycle, possibly with a separate high-frequency-mouseover event
> for those who need it.)
Right, I want to just fix this patch to match the existing behaviour, and then we can file a follow-up to reconsider how :hover handling works.
> Keep the old behavior of the code or the test?
Both. I want to make code changes so that the existing test continues to pass.
>
> Hmmm. I thought I'd concluded that those attempts had already been broken
> by other things being async, but maybe I'm misremembering.
With your patch to add back the reporting of multiple callbacks, we fail one step of the test currently. All the rest are working correctly.
The patch in this bug (as it stands) regresses this considerably.
>
> Does flushing not do that?
The main problem is that a style change in the parent document sets up a delayed resize on a child document.
We call FlushPendingNotifications on the parent document, but this doesn't flush delayed resizes on children.
I think we either need to recursively call FlushPendingNotifications on all descendant documents (what I tried locally), or include flushing delayed resizes on child documents as part of FlushPendingNotification. The latter is probably sufficient, and should be faster.
Comment 58•9 years ago
|
||
(In reply to Matt Woodrow (:mattwoodrow) from comment #59)
> Right, I want to just fix this patch to match the existing behaviour, and
> then we can file a follow-up to reconsider how :hover handling works.
ok
> > Does flushing not do that?
>
> The main problem is that a style change in the parent document sets up a
> delayed resize on a child document.
>
> We call FlushPendingNotifications on the parent document, but this doesn't
> flush delayed resizes on children.
>
> I think we either need to recursively call FlushPendingNotifications on all
> descendant documents (what I tried locally), or include flushing delayed
> resizes on child documents as part of FlushPendingNotification. The latter
> is probably sufficient, and should be faster.
I'd prefer to avoid having a flush in a parent document do stuff in child documents unless that's really needed. The right thing here does seem for this particular caller to flush child documents. And I certainly hope that flushing that child document flushes the delayed resizes in it.
Assignee | ||
Comment 59•9 years ago
|
||
Assignee: nobody → matt.woodrow
Assignee | ||
Comment 60•9 years ago
|
||
Flags: needinfo?(tnikkel)
Flags: needinfo?(dbaron)
Assignee | ||
Comment 61•9 years ago
|
||
Attachment #8345945 -
Attachment is obsolete: true
Attachment #8555922 -
Attachment is obsolete: true
Attachment #8687571 -
Attachment is obsolete: true
Attachment #8687575 -
Attachment is obsolete: true
Attachment #8741212 -
Flags: review+
Assignee | ||
Comment 62•9 years ago
|
||
This just removes part7() (since the double resize gets coalesced, and never fires an event), and then updates the numbering on all following tests. The diff makes it look a lot more complex.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=181bddb9a1f4
Assignee | ||
Updated•9 years ago
|
Attachment #8741210 -
Flags: review?(dbaron)
Assignee | ||
Updated•9 years ago
|
Attachment #8741211 -
Flags: review?(dbaron)
Assignee | ||
Updated•9 years ago
|
Attachment #8741213 -
Flags: review?(dbaron)
Updated•9 years ago
|
Attachment #8741210 -
Flags: review?(dbaron) → review+
Comment 63•9 years ago
|
||
Could you explain what it is that part 2 does? Why does it help? What does it fix?
(In particular, I'd like to understand both (a) why it doesn't need to be a FlushPendingNotifications like we do for the main document and (b) why we don't need to do anything for sub-sub-documents, etc.)
Flags: needinfo?(matt.woodrow)
Updated•9 years ago
|
Attachment #8741213 -
Flags: review?(dbaron) → review+
Assignee | ||
Comment 64•9 years ago
|
||
(In reply to David Baron [:dbaron] ⌚️UTC-7 from comment #65)
> Could you explain what it is that part 2 does? Why does it help? What does
> it fix?
>
> (In particular, I'd like to understand both (a) why it doesn't need to be a
> FlushPendingNotifications like we do for the main document and (b) why we
> don't need to do anything for sub-sub-documents, etc.)
The problem happens when we get a :hover triggered restyle in the parent document, that results in a change of size for the child document.
Our current attempts to flush all style changes in the parent don't include this resize, so it isn't effective in preventing :hover oscillations.
My understanding (which may be entirely incorrect) is that a :hover change in the parent can't cause any 'normal' restyle changes in the child, the only thing that can be affected is the size.
The same goes for further descendants, the :hover change in the parent can't cause restyling (or even resizes) for those descendants, so we don't need to do any flushing.
Flags: needinfo?(matt.woodrow) → needinfo?(dbaron)
Comment 65•9 years ago
|
||
(In reply to Matt Woodrow (:mattwoodrow) from comment #66)
> The problem happens when we get a :hover triggered restyle in the parent
> document, that results in a change of size for the child document.
>
> Our current attempts to flush all style changes in the parent don't include
> this resize, so it isn't effective in preventing :hover oscillations.
So you're saying that failing to flush a delayed resize in the child document means that we've failed to update the layout in the parent document? That doesn't seem right to me; I thought the iframe's size in the parent document should be up-to-date even if the child document inside that iframe has a delayed resize.
> My understanding (which may be entirely incorrect) is that a :hover change
> in the parent can't cause any 'normal' restyle changes in the child, the
> only thing that can be affected is the size.
So I don't see why the :hover change in the parent can't affect the :hover-ed content in the child document, which could in turn cause another layout change in the child document (although not in the parent).
This also feels a bit like an unusual case -- although it happens to be the mechanism test_hover uses to test a bunch of cases, which is why it comes up. That's also why I worry that this patch may be fixing test_hover without fixing the general problem.
> The same goes for further descendants, the :hover change in the parent can't
> cause restyling (or even resizes) for those descendants, so we don't need to
> do any flushing.
... except that what is in :hover inside those documents can change. (Though, again, that can only affect what's inside them and not the parent document -- but it still seems like it has to do with how we stop oscillations. Allowing one oscillation per level of nesting seems like a somewhat iffy strategy.)
Flags: needinfo?(dbaron) → needinfo?(matt.woodrow)
Assignee | ||
Comment 66•9 years ago
|
||
Ok, I debugged this again and I think I understand it better now.
PresShell::SynthesizeMouseMove on a sub document will re-call the function on the root pres shell instead [1].
This means the the synthesize mouse code, and the code that attempts to flush any changes as a result of it only ever runs for the root document.
So, I think you're right, we could actually have :hover oscillation happening in a child document (without resizing the document itself at all), and we'd current fail to prevent the oscillations.
We probably do need to do a full flush of all documents and descendants to make sure we prevent oscillations. I'll write a patch for that.
[1] http://mxr.mozilla.org/mozilla-central/source/layout/base/nsPresShell.cpp#5438
Flags: needinfo?(matt.woodrow)
Assignee | ||
Comment 67•9 years ago
|
||
Attachment #8741211 -
Attachment is obsolete: true
Attachment #8741211 -
Flags: review?(dbaron)
Attachment #8749031 -
Flags: review?(dbaron)
Comment 68•9 years ago
|
||
Comment on attachment 8749031 [details] [diff] [review]
Part 2: When flushing changes made by :hover style, make sure we also flush any pending changes on child documents
Maybe call it FlushLayoutRecursive instead of FlushNotificationsRecursive?
Also, local style puts "bool" on its own line and then FlushNotificationsRecursive starting at the beginning of the next.
Also maybe assert that aData is null?
r=dbaron with that, and thanks for putting up with my repeatedly-delayed requests
Attachment #8749031 -
Flags: review?(dbaron) → review+
Assignee | ||
Comment 69•9 years ago
|
||
Looks like we still need this.
This test only fails on OSX 10.10 in automation, I can't reproduce it locally at all, so I don't have many details on exactly why we need to wait for the scroll event.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7654ae94eb45
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3b0215a273e4
Attachment #8749875 -
Flags: review?(tnikkel)
Comment 70•9 years ago
|
||
Comment on attachment 8749875 [details] [diff] [review]
Rebased Mats' patch for fix test_bug602962.xu
>Bug 881832 - Fix test_bug602962.xul and disable test_hover.html. r=tn
You don't want the "and disable test_hover.html" in the commit message anymore.
Assignee | ||
Comment 71•9 years ago
|
||
(In reply to David Baron [:dbaron] ⌚️UTC-7 (review requests must explain patch) (busy May 9-13) from comment #72)
> Comment on attachment 8749875 [details] [diff] [review]
> Rebased Mats' patch for fix test_bug602962.xu
>
> >Bug 881832 - Fix test_bug602962.xul and disable test_hover.html. r=tn
>
> You don't want the "and disable test_hover.html" in the commit message
> anymore.
Good catch, thanks!
Comment 72•9 years ago
|
||
Comment on attachment 8749875 [details] [diff] [review]
Rebased Mats' patch for fix test_bug602962.xu
Looks like the resize event can be fired at the start of PresShell::FlushPendingNotifications before reflow etc, so it's possible that the frames don't have the new size yet when the resize event has fired. That would make the scrollBy call a no-op since there is no scrollable overflow.
So perhaps you just need to do the scrollBy on a setTimeout? The scrolling should be instant as far as I can tell. If that doesn't work you have r+ to land this version.
Attachment #8749875 -
Flags: review?(tnikkel) → review+
Comment 73•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/d711b7c19a43
https://hg.mozilla.org/integration/mozilla-inbound/rev/11106afdcbe7
https://hg.mozilla.org/integration/mozilla-inbound/rev/6b3c5e185b04
https://hg.mozilla.org/integration/mozilla-inbound/rev/8a0d5df41cfb
https://hg.mozilla.org/integration/mozilla-inbound/rev/117e8e24d714
Comment 74•9 years ago
|
||
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/b02eddb72e693ff4b4f6441a50609b439c416eac for timeouts in devtools/client/shared/test/browser_html_tooltip-02.js like https://treeherder.mozilla.org/logviewer.html#?job_id=27709700&repo=mozilla-inbound
Updated•9 years ago
|
Flags: needinfo?(matt.woodrow)
Assignee | ||
Comment 75•9 years ago
|
||
This test failure was a huge pain, entirely intermittent, only happens in opt builds. Yay for rr!
At the point when we call synthesizeMouseAtCenter, the newly added content hasn't yet been reflowed, so hit testing fails (new content has an empty area initially), we never fire the "click" event and the test hangs.
It looks like we wait for the HTMLTooltip to fire "shown", but this happens from a setTimeout [1] rather than a 'real' event.
Julian, should the "shown" event be changed, or would you prefer to just fix the test? We should be able to just query layout on the new content to force it to reflow before we fire the mouse event.
[1] http://mxr.mozilla.org/mozilla-central/source/devtools/client/shared/widgets/HTMLTooltip.js#136
Flags: needinfo?(matt.woodrow) → needinfo?(jdescottes)
Comment 76•9 years ago
|
||
Ah, sorry about that!
This patch waits for reflow&repaint in the showTooltip & hideTooltip helpers used in the HTMLTooltip tests. Let me know if that could work for the moment.
> Julian, should the "shown" event be changed,
> or would you prefer to just fix the test
I would actually prefer to change the event so that we don't risk reintroducing flaky tests. How can we do this?
I am reluctant to trigger a mandatory reflow before emitting "shown", because I feel like this will only ever be useful for test code?
Is there any other way to handle this?
Flags: needinfo?(jdescottes)
Attachment #8752818 -
Flags: feedback?(matt.woodrow)
Assignee | ||
Comment 77•9 years ago
|
||
Comment on attachment 8752818 [details] [diff] [review]
bug881832.htmltooltip-fix-tests.patch
Review of attachment 8752818 [details] [diff] [review]:
-----------------------------------------------------------------
Looks like that works.
The best way to change the shown event would probably be to wait for the MozAfterPaint event.
Attachment #8752818 -
Flags: feedback?(matt.woodrow) → feedback+
Assignee | ||
Comment 78•9 years ago
|
||
Julian, are you ok with landing your current fix in the interim?
Flags: needinfo?(jdescottes)
Comment 79•9 years ago
|
||
(In reply to Matt Woodrow (:mattwoodrow) from comment #80)
> Julian, are you ok with landing your current fix in the interim?
Sure. I logged a separate bug to land the test fixes and set it as a blocker for this one. Patch is up for review.
Flags: needinfo?(jdescottes)
Assignee | ||
Comment 80•9 years ago
|
||
This fails intermittently on OSX, and I can't reproduce it.
It's a fuzz of 1bit across some of the dotted borders, I guess we're painting an extra time with the delay and it's blending slightly differently?
Getting rid of the dotted border fixes it, and shouldn't affect the purpose of the test at all.
Attachment #8754633 -
Flags: review?(tnikkel)
Updated•9 years ago
|
Attachment #8754633 -
Flags: review?(tnikkel) → review+
Comment 81•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/8ee1824f2a57
https://hg.mozilla.org/integration/mozilla-inbound/rev/7fe79f07189f
https://hg.mozilla.org/integration/mozilla-inbound/rev/95efd250e29f
https://hg.mozilla.org/integration/mozilla-inbound/rev/779f5336b81e
https://hg.mozilla.org/integration/mozilla-inbound/rev/32e01c144cd4
https://hg.mozilla.org/integration/mozilla-inbound/rev/fac50ce10b07
Comment 82•9 years ago
|
||
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/d63acfaf8820 for too-frequent failures in a devtools mochitest-chrome test like https://treeherder.mozilla.org/logviewer.html#?job_id=28476575&repo=mozilla-inbound - your best platform for reproducing it is WinXP opt (6 out of 10 runs on your push), though with enough effort you can also hit it on OS X opt, and all three flavors of Windows PGO.
Comment 83•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/8e3f2c7b9039
https://hg.mozilla.org/integration/mozilla-inbound/rev/d515b3ab1736
https://hg.mozilla.org/integration/mozilla-inbound/rev/26e22ea9e8dd
https://hg.mozilla.org/integration/mozilla-inbound/rev/7264fc82460f
https://hg.mozilla.org/integration/mozilla-inbound/rev/a5acaabd87d5
https://hg.mozilla.org/integration/mozilla-inbound/rev/759d94861665
Updated•9 years ago
|
status-firefox48:
--- → affected
status-firefox49:
--- → affected
Comment 84•9 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/8e3f2c7b9039
https://hg.mozilla.org/mozilla-central/rev/d515b3ab1736
https://hg.mozilla.org/mozilla-central/rev/26e22ea9e8dd
https://hg.mozilla.org/mozilla-central/rev/7264fc82460f
https://hg.mozilla.org/mozilla-central/rev/a5acaabd87d5
https://hg.mozilla.org/mozilla-central/rev/759d94861665
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Updated•8 years ago
|
Updated•8 years ago
|
Updated•8 years ago
|
Depends on: CVE-2016-9905
Updated•8 years ago
|
Updated•8 years ago
|
status-firefox-esr45:
--- → affected
tracking-firefox-esr45:
--- → 50+
Comment on attachment 8741210 [details] [diff] [review]
Part 1: Specify whether the height has changed when calling PresShell::ResizeReflow
We need this in ESR, see bug 928187.
This should land for 45.5.0esr. The release date for Firefox and ESR has changed, and is now Nov. 15.
Attachment #8741210 -
Flags: approval-mozilla-esr45+
Attachment #8749031 -
Flags: approval-mozilla-esr45+
Attachment #8741212 -
Flags: approval-mozilla-esr45+
Attachment #8741213 -
Flags: approval-mozilla-esr45+
Matt, is that everything? Should we also uplift the text fixes? Thanks.
Flags: needinfo?(matt.woodrow)
Assignee | ||
Comment 87•8 years ago
|
||
(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #88)
> Matt, is that everything? Should we also uplift the text fixes? Thanks.
I think you'll need to uplift the test fixes to keep everything green.
Flags: needinfo?(matt.woodrow)
Assignee | ||
Comment 89•8 years ago
|
||
Rebased queue pushed to try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0eeedef9d31f9b0d09648b3c43d13ecbcb01dbeb
Flags: needinfo?(matt.woodrow)
Comment 90•8 years ago
|
||
matt, seems this is still waiting for the rebase ?
Flags: needinfo?(matt.woodrow)
Comment 91•8 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-esr45/rev/1123263318a3
https://hg.mozilla.org/releases/mozilla-esr45/rev/dc87c0a39adf
https://hg.mozilla.org/releases/mozilla-esr45/rev/f20e5f488368
https://hg.mozilla.org/releases/mozilla-esr45/rev/7950c4d5bd7c
https://hg.mozilla.org/releases/mozilla-esr45/rev/972734ec21b6
Flags: in-testsuite+
Updated•8 years ago
|
Flags: needinfo?(matt.woodrow)
Comment 92•8 years ago
|
||
This broke binary compatibility on ESR45, which Thunderbird is committed to maintain throughout the 45 series. Bug 1323592 will provide a patch on our branch THUNDERBIRD_45_VERBRANCH to restore binary compatibility.
Blocks: 1323592
Updated•7 years ago
|
Depends on: CVE-2017-7828
You need to log in
before you can comment on or make changes to this bug.
Description
•