Closed Bug 881832 Opened 11 years ago Closed 9 years ago

Make inner document reflow asynchronous to improve performance

Categories

(Core :: Layout, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox45 - wontfix
firefox46 + wontfix
firefox47 + wontfix
firefox48 --- wontfix
firefox49 --- fixed
firefox-esr45 50+ fixed

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+
Details | Diff | Splinter Review
(deleted), patch
mattwoodrow
: review+
Details | Diff | Splinter Review
(deleted), patch
dbaron
: review+
Details | Diff | Splinter Review
(deleted), patch
dbaron
: review+
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
Attached file stack (deleted) —
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)
Attached patch fix (obsolete) (deleted) — Splinter Review
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)
Attached patch fix tests, except one (obsolete) (deleted) — Splinter Review
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.
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
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.
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.
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
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
Attached patch fix, v3 (obsolete) (deleted) — Splinter Review
Bumped uuids and fixed nits.
Attachment #761071 - Attachment is obsolete: true
Attachment #8345942 - Flags: review+
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 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-
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)
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)
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).
(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)
Attached patch unbitrotted (obsolete) (deleted) — Splinter Review
Just stashing a copy of this here to avoid having to unbitrot again.
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)
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?
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)
Attached patch unbitrotted (obsolete) (deleted) — Splinter Review
Attachment #8384894 - Attachment is obsolete: true
Can we just disable the test and land this if nobody has time to fix the test?
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)
Attached patch unbitrotted (obsolete) (deleted) — Splinter Review
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.
Attached patch unbitrotted (obsolete) (deleted) — Splinter Review
Attachment #8509706 - Attachment is obsolete: true
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.
Attached patch unbitrotted (obsolete) (deleted) — Splinter Review
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
Attached file printf debug, without delayed resizes (deleted) —
Attached file printf debug, with delayed resizes (deleted) —
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.
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.
Attachment #8555915 - Attachment is patch: false
Attachment #8555396 - Attachment is patch: true
Attached patch debug printfs, fwiw (obsolete) (deleted) — Splinter Review
Attached patch unbitrotted (obsolete) (deleted) — Splinter Review
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)
Attached patch Rebased fix (obsolete) (deleted) — Splinter Review
Attachment #8345942 - Attachment is obsolete: true
Attachment #8591363 - Attachment is obsolete: true
Attached patch hover_helper tweaks (obsolete) (deleted) — Splinter Review
(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)
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
> (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.
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.
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?
(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.
Flags: needinfo?(tnikkel)
Flags: needinfo?(dbaron)
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+
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
Attachment #8741210 - Flags: review?(dbaron)
Attachment #8741211 - Flags: review?(dbaron)
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+
(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)
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)
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+
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.
(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+
Flags: needinfo?(matt.woodrow)
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)
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)
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+
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)
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+
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.
Depends on: 1279202
No longer blocks: 1278903
Depends on: 1278903
Depends on: CVE-2016-9905
No longer blocks: 1305283
Depends on: 1305283
No longer depends on: 1305283
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+
Matt, is that everything? Should we also uplift the text fixes? Thanks.
Flags: needinfo?(matt.woodrow)
(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)
matt, seems this is still waiting for the rebase ?
Flags: needinfo?(matt.woodrow)
Flags: needinfo?(matt.woodrow)
Depends on: 1321867
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
Depends on: 1371920
Depends on: CVE-2017-7828
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: