Make inner document reflow asynchronous to improve performance

RESOLVED FIXED in Firefox 49

Status

()

Core
Layout
RESOLVED FIXED
5 years ago
2 months ago

People

(Reporter: mats, Assigned: mattwoodrow)

Tracking

(Depends on: 1 bug, {perf})

Trunk
mozilla49
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox45- wontfix, firefox46+ wontfix, firefox47+ wontfix, firefox48 wontfix, firefox49 fixed, firefox-esr4550+ fixed)

Details

Attachments

(11 attachments, 14 obsolete attachments)

17.38 KB, text/plain
Details
6.91 KB, text/plain
Details
12.31 KB, text/plain
Details
9.47 KB, text/plain
Details
7.87 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
23.51 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
7.25 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
2.13 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
2.63 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
4.52 KB, patch
mattwoodrow
: feedback+
Details | Diff | Splinter Review
1.52 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
(Reporter)

Description

5 years ago
Created attachment 761065 [details]
stack

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

5 years ago
Created attachment 761071 [details] [diff] [review]
fix

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

5 years ago
Created attachment 761075 [details] [diff] [review]
fix tests, except one

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

5 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

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

5 years ago
Thanks!  All three tests pass unmodified with that patch in my tree.  Nice!

https://tbpl.mozilla.org/?tree=Try&rev=ec4e8271d5c8
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

5 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"
That is surprising. FWIW the patch from bug 641188 is green https://tbpl.mozilla.org/?tree=Try&rev=0ddb5fcf8458
Blocks: 881087
Blocks: 928187
No longer blocks: 881087
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+
Mats, have you had a chance to look at this?
Assignee: nobody → matspal
(Reporter)

Comment 13

4 years ago
Created attachment 8345942 [details] [diff] [review]
fix, v3

Bumped uuids and fixed nits.
Attachment #761071 - Attachment is obsolete: true
Attachment #8345942 - Flags: review+
(Reporter)

Comment 14

4 years ago
Created attachment 8345945 [details] [diff] [review]
Fix test_bug602962.xul and disable test_hover.html

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

4 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 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 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

4 years ago
Perhaps it's better if someone else takes a fresh look at the remaining issue...
Assignee: matspal → nobody
Jet, is there somebody who can look at this?
Flags: needinfo?(bugs)

Comment 20

4 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

4 years ago
Flags: needinfo?(bugs)
I rebased and pushed to try to see where we stand now
https://tbpl.mozilla.org/?tree=Try&rev=fe38461fd169
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

4 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)
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)
Created attachment 8384894 [details] [diff] [review]
unbitrotted

Just stashing a copy of this here to avoid having to unbitrot again.
(Reporter)

Comment 26

4 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)
(Clearing needinfo. Unlikely to have time to dig deeper into this in the short term.)
Flags: needinfo?(tnikkel)

Updated

4 years ago
Flags: needinfo?(tnikkel)
Flags: needinfo?(matspal)
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 30

4 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 31

4 years ago
Created attachment 8418975 [details] [diff] [review]
unbitrotted
Attachment #8384894 - Attachment is obsolete: true
Can we just disable the test and land this if nobody has time to fix the test?

Comment 33

3 years ago
David: do you agree with Mats' findings? ie. that our synthetic mouse hover is faulty?
Flags: needinfo?(dbaron)
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 35

3 years ago
Created attachment 8509706 [details] [diff] [review]
unbitrotted

Updated for recent directory changes.  This applies cleanly to latest
m-i (rev 0f6aedc8d6e4).
Attachment #8418975 - Attachment is obsolete: true
Flags: needinfo?(dbaron)
Any update here?  This bug is about a year and a half old now.
(Reporter)

Comment 38

3 years ago
Created attachment 8550816 [details] [diff] [review]
unbitrotted

Applies to m-c 35df417b93a7.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2a48aa9f968b
Attachment #8509706 - Attachment is obsolete: true
(Reporter)

Comment 39

3 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 40

3 years ago
Created attachment 8555396 [details] [diff] [review]
unbitrotted

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 41

3 years ago
Created attachment 8555908 [details]
printf debug, without delayed resizes
(Reporter)

Comment 42

3 years ago
Created attachment 8555912 [details]
printf debug, with delayed resizes

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 43

3 years ago
Created attachment 8555915 [details]
a couple of stacks that might be of interest (with delayed resizes patch)

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

3 years ago
Attachment #8555915 - Attachment is patch: false
(Reporter)

Updated

3 years ago
Attachment #8555396 - Attachment is patch: true
(Reporter)

Comment 44

3 years ago
Created attachment 8555922 [details] [diff] [review]
debug printfs, fwiw
(Reporter)

Comment 45

3 years ago
Created attachment 8591363 [details] [diff] [review]
unbitrotted

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
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...)
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.
... 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
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 50

2 years ago
Created attachment 8687571 [details] [diff] [review]
Rebased fix
Attachment #8345942 - Attachment is obsolete: true
Attachment #8591363 - Attachment is obsolete: true
(Reporter)

Comment 51

2 years ago
Created attachment 8687575 [details] [diff] [review]
hover_helper tweaks

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

2 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 53

2 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: --- → +
Not tracking, too late for 45.
status-firefox45: affected → wontfix
tracking-firefox45: ? → -
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.
status-firefox46: affected → wontfix
(Assignee)

Comment 57

2 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
(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 59

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

2 years ago
Created attachment 8741210 [details] [diff] [review]
Part 1: Specify whether the height has changed when calling PresShell::ResizeReflow
Assignee: nobody → matt.woodrow
(Assignee)

Comment 62

2 years ago
Created attachment 8741211 [details] [diff] [review]
Part 2: When flushing changes made by :hover style, make sure we also flush any pending resizes on child documents
Flags: needinfo?(tnikkel)
Flags: needinfo?(dbaron)
(Assignee)

Comment 63

2 years ago
Created attachment 8741212 [details] [diff] [review]
Part 3: Make inner document reflow asynchronous.
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 64

2 years ago
Created attachment 8741213 [details] [diff] [review]
Part 4: Fix hover_helper to handle that multiple resizes due to :hover oscillation are now coalesced and don't fire resize events.

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

2 years ago
Attachment #8741210 - Flags: review?(dbaron)
(Assignee)

Updated

2 years ago
Attachment #8741211 - Flags: review?(dbaron)
(Assignee)

Updated

2 years ago
Attachment #8741213 - Flags: review?(dbaron)
Attachment #8741210 - Flags: review?(dbaron) → review+
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)
Attachment #8741213 - Flags: review?(dbaron) → review+
(Assignee)

Comment 66

2 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)
(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 68

2 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 69

2 years ago
Created 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
Attachment #8741211 - Attachment is obsolete: true
Attachment #8741211 - Flags: review?(dbaron)
Attachment #8749031 - Flags: review?(dbaron)
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 71

2 years ago
Created attachment 8749875 [details] [diff] [review]
Rebased Mats' patch for fix test_bug602962.xu

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 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 73

2 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 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 75

2 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
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
Flags: needinfo?(matt.woodrow)
(Assignee)

Comment 77

2 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)
Created attachment 8752818 [details] [diff] [review]
bug881832.htmltooltip-fix-tests.patch

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 79

2 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 80

2 years ago
Julian, are you ok with landing your current fix in the interim?
Flags: needinfo?(jdescottes)
Depends on: 1273827
(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 82

2 years ago
Created attachment 8754633 [details] [diff] [review]
Change resize-reflow-001.inner.html

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)
Attachment #8754633 - Flags: review?(tnikkel) → review+

Comment 83

2 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
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 85

2 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
status-firefox47: affected → wontfix
status-firefox48: --- → affected
status-firefox49: --- → affected

Comment 86

2 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
Last Resolved: 2 years ago
status-firefox49: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49

Updated

2 years ago
Depends on: 1279202
status-firefox48: affected → wontfix
Blocks: 1278903
No longer blocks: 1278903
Depends on: 1278903

Updated

a year ago
Depends on: 1293985
Blocks: 1305283
No longer blocks: 1305283
Depends on: 1305283
No longer depends on: 1305283
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 89

a year 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)
This needs rebasing for esr45 uplift.
Flags: needinfo?(matt.woodrow)
(Assignee)

Comment 91

a year ago
Rebased queue pushed to try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0eeedef9d31f9b0d09648b3c43d13ecbcb01dbeb
Flags: needinfo?(matt.woodrow)
matt, seems this is still waiting for the rebase ?
Flags: needinfo?(matt.woodrow)

Comment 93

a year ago
bugherderuplift
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
status-firefox-esr45: affected → fixed
Flags: in-testsuite+
Flags: needinfo?(matt.woodrow)

Updated

a year ago
Depends on: 1321867
(Reporter)

Updated

a year ago
Depends on: 1163212
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

6 months ago
Depends on: 1371920

Updated

2 months ago
Depends on: 1406750
You need to log in before you can comment on or make changes to this bug.