Closed
Bug 244366
Opened 20 years ago
Closed 20 years ago
[FIXr]Invalidate event processing should flush reflows
Categories
(Core :: Web Painting, defect, P1)
Tracking
()
RESOLVED
FIXED
mozilla1.8beta1
People
(Reporter: bzbarsky, Assigned: bzbarsky)
References
Details
Attachments
(2 files, 3 obsolete files)
359 bytes,
text/html
|
Details | |
40.27 KB,
patch
|
roc
:
review+
roc
:
superreview+
|
Details | Diff | Splinter Review |
The short story is that the fix for bug 36849 introduced a hack where we flush reflows and repaints on mousemove events to prevent flickering. It seems like a more comprehensive solution would be as following (in the viewmanager): 1) We get ready to process invalidates 2) We tell the presshell to flush reflows (which incidentally will do all its invalidating in a view batch, if we do it right). 3) We process our invalidate queue. During step 2, posting of an invalidate event is suppressed (since we're about to process one anyway). One possible issue with this approach is that it may reduce responsiveness... Would need to test.
Assignee | ||
Updated•20 years ago
|
Target Milestone: --- → mozilla1.8alpha6
Assignee | ||
Comment 1•20 years ago
|
||
This is quite raw; the event state manager change, in particular, is just wrong.
Assignee | ||
Comment 2•20 years ago
|
||
There's a problem here, by the way... we could flush reflows during invalidate processing, but new reflows could happen before we go to paint, without the new invalidates having happened yet. So we really want to flush all the reflows and invalidates when we get the paint event from widget or something... Except that call gives us the area to paint, and that area would need to be changed in the call itself. Robert, am I missing something, or does that sum up what needs to happen?
Target Milestone: mozilla1.8alpha6 → mozilla1.9alpha
Assignee | ||
Comment 3•20 years ago
|
||
Assignee | ||
Updated•20 years ago
|
Attachment #170634 -
Attachment is obsolete: true
Assignee | ||
Comment 4•20 years ago
|
||
Summary of changes: 1) Add a WillPaint notification to nsIViewObserver. 2) Call this from two places: FlushPendingInvalidates, in the hope that this will coalesce the invalidates we already have pending and the invalidates the reflow will generate, and WM_PAINT event handling. The latter is definitely needed to solve some jitter and blinking problems we had. 3) Remove the FlushPendingEvents calls we had on every mousemove (but keep them while we're in a drag-drop gesture). 4) Change presshell code that could tear down the presshell from inside ProcessReflowCommands to not do that (add a check like the nsObjectFrame code had, and consolidate the identical code in a prescontext method). I've been meaning to fix this anyway, since it could cause crashes; these just became more likely when we were reflowing from inside ESM code and WM_PAINT code and such. I put the method on nsPresContext instead of nsIPresShell because that's a more "layout-private" class as far as I can tell, but I don't have a strong attachment to this placement.
Attachment #170786 -
Attachment is obsolete: true
Attachment #170800 -
Flags: superreview?(dbaron)
Attachment #170800 -
Flags: review?(roc)
Assignee | ||
Comment 5•20 years ago
|
||
I's sort of like to get this in for beta, but it's a little scary...
Priority: -- → P1
Summary: Invalidate event processing should flush reflows → [FIX]Invalidate event processing should flush reflows
Target Milestone: mozilla1.9alpha → mozilla1.8beta
Comment 6•20 years ago
|
||
With the Proposed patch, I crash by just following bug 277653. Without it, I don't crash with my Firefox non-debug build. The testcase and the url in bug 277653 work nice, though. :)
Assignee | ||
Comment 7•20 years ago
|
||
Martijn, what are the exact steps to reproduce that crash you're seeing? Can you get a stack, possibly? Either put that info in this bug or mail me?
Assignee | ||
Comment 8•20 years ago
|
||
Comment on attachment 170800 [details] [diff] [review] Proposed patch This has a problem (the one Martijn pointed out; the upshot is that editor actually forces layout and paint from inside frame construction, so we end up reflowing in the middle of frame construction, which is bad).
Attachment #170800 -
Flags: superreview?(dbaron)
Attachment #170800 -
Flags: superreview-
Attachment #170800 -
Flags: review?(roc)
Attachment #170800 -
Flags: review-
Comment 9•20 years ago
|
||
This is a minimal testcase that makes Mozilla crash, when you've applied the "Proposed patch" patch.
Assignee | ||
Comment 10•20 years ago
|
||
Changes, in addition to those listed in comment 4: 5) Make editor end its view update batch as DEFERRED when it's not doing sync reflow/painting. This is what actually fixes the crash Martijn ran into (which I didn't see in my build because it had unrelated text control changes). The comment here explains why this is the right thing to do in any case. 6) Fix a bug in view/viewmanager where we'd bail out of nsView::ResetWidgetBounds on the assumption that the end of the batch would process the change, but didn't let the viewmanager know that it had pending changes. This caused tooltips to not appear, for example, since with this patch we always check mHasPendingUpdates (which I have accordingly renamed) before calling ProcessPendingUpdates. 7) Even though the editor change fixed the crash Martijn saw, I decided to add some more bulletproofing and not reflow on paint while in the middle of things bracketed by WillCauseReflow/DidCauseReflow (mostly frame construction; the other stuff bracketed by these was already protected by mIsReflowing). While I was there, I also disabled flushing reflows in the same circumstances. I've tested this on a bunch of DHTML testcases now, as well as on various normal pages, with and without plugins, in a vanilla build with nothing but this patch applied. So far, so good. I'll keep testing it, just in case....
Attachment #170800 -
Attachment is obsolete: true
Attachment #171103 -
Flags: superreview?(dbaron)
Attachment #171103 -
Flags: review?(roc)
Comment on attachment 171103 [details] [diff] [review] Slightly more bulletproofed go for it.
Attachment #171103 -
Flags: superreview?(dbaron)
Attachment #171103 -
Flags: superreview+
Attachment #171103 -
Flags: review?(roc)
Attachment #171103 -
Flags: review+
Assignee | ||
Updated•20 years ago
|
Summary: [FIX]Invalidate event processing should flush reflows → [FIXr]Invalidate event processing should flush reflows
Assignee | ||
Comment 12•20 years ago
|
||
Checked in. The only effect on perf metrics was that Tdhtml went up a bit. I looked into it, and a few of the tests got slower (the rest did not). Testing in a patched and unpatched build, all the tests that got slower were skipping a lot more animation frames without this patch -- this patch makes the animation much smoother.
Status: NEW → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 13•20 years ago
|
||
This seems to have caused at least bug 279308 and bug 279205.
Assignee | ||
Comment 14•20 years ago
|
||
Bug 279205 is probably caused by this change to nsViewManager in this patch: - nsCOMPtr<nsIViewObserver> vobs; - vVM->GetViewObserver(*getter_AddRefs(vobs)); + // Hold a refcount to the observer. The continued existence of the observer will + // delay deletion of this view hierarchy should the event want to cause its + // destruction in, say, some JavaScript event handler. + nsCOMPtr<nsIViewObserver> vobs = GetViewObserver(); Note that this gets the wrong view observer.
Assignee | ||
Comment 15•20 years ago
|
||
This also caused bug 279786
Comment 16•20 years ago
|
||
The fix seems to have caused bug #280214 as well.
Assignee | ||
Comment 17•20 years ago
|
||
Adding regressions caused or maybe caused by this to the "depends on" list.
Comment 18•20 years ago
|
||
This patch may have caused bug 281173
Comment 19•19 years ago
|
||
I believe this patch caused a regression in the mozilla application suite and for Thunderbird regarding excessive black flicker. I narrowed the regression window down to the day this change went in. See Bug #283123 for details.
Assignee | ||
Updated•19 years ago
|
Updated•6 years ago
|
Component: Layout: View Rendering → Layout: Web Painting
You need to log in
before you can comment on or make changes to this bug.
Description
•