881832 - Make inner document reflow asynchronous to improve performance

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Mats Palmgren (inactive)]

Attachment: 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)

Comment 1

[Mats Palmgren (inactive)]

Attachment: 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"

Comment 2

[Mats Palmgren (inactive)]

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

Comment 3

[Timothy Nikkel (:tnikkel)]

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.

Comment 4

[Mats Palmgren (inactive)]

Comment on attachment 761075 [details] [diff] [review]
fix tests, except one

OK, "it should just work" works for me ;-)

Comment 5

[Mats Palmgren (inactive)]

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

[Timothy Nikkel (:tnikkel)]

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.

Comment 7

[Mats Palmgren (inactive)]

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

https://tbpl.mozilla.org/?tree=Try&rev=ec4e8271d5c8

Comment 8

[Timothy Nikkel (:tnikkel)]

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.

Comment 9

[Mats Palmgren (inactive)]

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

[Timothy Nikkel (:tnikkel)]

That is surprising. FWIW the patch from bug 641188 is green https://tbpl.mozilla.org/?tree=Try&rev=0ddb5fcf8458

Comment 11

[Timothy Nikkel (:tnikkel)]

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.

Comment 12

[Andrew McCreight [:mccr8]]

Mats, have you had a chance to look at this?

Comment 13

[Mats Palmgren (inactive)]

Attachment: fix, v3

Bumped uuids and fixed nits.

Comment 14

[Mats Palmgren (inactive)]

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

Comment 15

[Mats Palmgren (inactive)]

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?

Comment 16

[Timothy Nikkel (: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 17

[Timothy Nikkel (:tnikkel)]

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.

Comment 18

[Mats Palmgren (inactive)]

Perhaps it's better if someone else takes a fresh look at the remaining issue...

Comment 19

[Andrew McCreight [:mccr8]]

Jet, is there somebody who can look at this?

Comment 20

[Jet Villegas (inactive)]

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?

Comment 21

[Timothy Nikkel (:tnikkel)]

I rebased and pushed to try to see where we stand now
https://tbpl.mozilla.org/?tree=Try&rev=fe38461fd169

Comment 22

[Timothy Nikkel (:tnikkel)]

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

Comment 23

[Mats Palmgren (inactive)]

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

Comment 24

[Timothy Nikkel (:tnikkel)]

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.

Comment 25

[Timothy Nikkel (:tnikkel)]

Attachment: unbitrotted

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

Comment 26

[Mats Palmgren (inactive)]

I tried that, doing a FlushDelayedResize(false) in the else-part too
under similar conditions.  It didn't help, test_hover.html still failed.

Comment 27

[Timothy Nikkel (:tnikkel)]

(Clearing needinfo. Unlikely to have time to dig deeper into this in the short term.)

Comment 29

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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?

Comment 30

[Mats Palmgren (inactive)]

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.

Comment 31

[Mats Palmgren (inactive)]

Attachment: unbitrotted

Comment 32

[Andrew McCreight [:mccr8]]

Can we just disable the test and land this if nobody has time to fix the test?

Comment 33

[Jet Villegas (inactive)]

David: do you agree with Mats' findings? ie. that our synthetic mouse hover is faulty?

Comment 34

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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?

Comment 35

[Mats Palmgren (inactive)]

Attachment: unbitrotted

Updated for recent directory changes.  This applies cleanly to latest
m-i (rev 0f6aedc8d6e4).

Comment 36

[Andrew McCreight [:mccr8]]

Any update here?  This bug is about a year and a half old now.

Comment 38

[Mats Palmgren (inactive)]

Attachment: unbitrotted

Applies to m-c 35df417b93a7.

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

Comment 39

[Mats Palmgren (inactive)]

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.

Comment 40

[Mats Palmgren (inactive)]

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

Comment 41

[Mats Palmgren (inactive)]

Attachment: printf debug, without delayed resizes

Comment 42

[Mats Palmgren (inactive)]

Attachment: 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.

Comment 43

[Mats Palmgren (inactive)]

Attachment: 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.

Comment 44

[Mats Palmgren (inactive)]

Attachment: debug printfs, fwiw

Comment 45

[Mats Palmgren (inactive)]

Attachment: unbitrotted

Updated the patch to see if Olli's patch in bug 1149555
might help with the remaining test failure.  It didn't.

Comment 46

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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 47

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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 48

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 50

[Mats Palmgren (inactive)]

Attachment: Rebased fix

Comment 51

[Mats Palmgren (inactive)]

Attachment: 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.)

Comment 52

[Mats Palmgren (inactive)]

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

Comment 53

[Mats Palmgren (inactive)]

> (This is on top of your fix in comment 22.)

Sorry, I meant on top of your fix in comment 48.

Comment 54

[Liz Henry (:lizzard) (relman/hg->git project)]

Mats are you still working on this? 
Tracking for 46+ since other bugs appear to depend on this, and it still affects current versions.

Comment 55

[Sylvestre Ledru [:Sylvestre]]

Not tracking, too late for 45.

Comment 56

[Liz Henry (:lizzard) (relman/hg->git project)]

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.

Comment 57

[Matt Woodrow (:mattwoodrow)]

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 58

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 59

[Matt Woodrow (:mattwoodrow)]

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

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 61

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 1: Specify whether the height has changed when calling PresShell::ResizeReflow

Comment 62

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 2: When flushing changes made by :hover style, make sure we also flush any pending resizes on child documents

Comment 63

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 3: Make inner document reflow asynchronous.

Comment 64

[Matt Woodrow (:mattwoodrow)]

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

Comment 65

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 66

[Matt Woodrow (:mattwoodrow)]

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

Comment 67

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 68

[Matt Woodrow (:mattwoodrow)]

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

Comment 69

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 2: When flushing changes made by :hover style, make sure we also flush any pending changes on child documents

Comment 70

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 71

[Matt Woodrow (:mattwoodrow)]

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

Comment 72

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 73

[Matt Woodrow (:mattwoodrow)]

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

[Timothy Nikkel (:tnikkel)]

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.

Comment 75

[Pulsebot]

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 76

[Phil Ringnalda (:philor)]

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

Comment 77

[Matt Woodrow (:mattwoodrow)]

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

Comment 78

[Julian Descottes [:jdescottes]]

Attachment: 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?

Comment 79

[Matt Woodrow (:mattwoodrow)]

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.

Comment 80

[Matt Woodrow (:mattwoodrow)]

Julian, are you ok with landing your current fix in the interim?

Comment 81

[Julian Descottes [:jdescottes]]

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

Comment 82

[Matt Woodrow (:mattwoodrow)]

Attachment: 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.

Comment 83

[Pulsebot]

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 84

[Phil Ringnalda (:philor)]

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

[Pulsebot]

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

Comment 86

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 87

[Liz Henry (:lizzard) (relman/hg->git project)]

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.

Comment 88

[Liz Henry (:lizzard) (relman/hg->git project)]

Matt, is that everything? Should we also uplift the text fixes? Thanks.

Comment 89

[Matt Woodrow (:mattwoodrow)]

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

Comment 90

[Ryan VanderMeulen [:RyanVM]]

This needs rebasing for esr45 uplift.

Comment 91

[Matt Woodrow (:mattwoodrow)]

Rebased queue pushed to try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0eeedef9d31f9b0d09648b3c43d13ecbcb01dbeb

Comment 92

[Carsten Book [:Tomcat]]

matt, seems this is still waiting for the rebase ?

Comment 93

[Ryan VanderMeulen [:RyanVM]]

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

Comment 94

[Kent James (:rkent)]

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.