1029982 - Don't change mNeedLayoutFlush and mNeedStyleFlush when nsPresShell::FlushPendingNotification failed

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Per bug 9870404 comment 86, we shouldn't change those values if the flush failed.

Comment 1

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Should be bug 987040....not 9870404

Comment 2

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: layout_not_flush v1

Let FlushPendingNotification return a boolean value which indicate whether flush success or not and not change mNeedLayoutFlush/mNeedStyleFlush if the flush failed.

Comment 3

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

try push: https://tbpl.mozilla.org/?tree=Try&rev=dd97d2207407

Comment 4

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

Comment on attachment 8445707 [details] [diff] [review]
layout_not_flush v1

>@@ -7816,24 +7816,25 @@ nsDocument::FlushPendingNotifications(mo
>   // already but the presshell hasn't actually done the corresponding work yet.
>   // So if mInFlush and reentering this code, we need to flush the presshell.
>   if (mNeedStyleFlush ||
>       (mNeedLayoutFlush && aType >= Flush_InterruptibleLayout) ||
>       aType >= Flush_Display ||
>       mInFlush) {
>     nsCOMPtr<nsIPresShell> shell = GetShell();
>     if (shell) {
>-      mNeedStyleFlush = false;
>-      mNeedLayoutFlush = mNeedLayoutFlush && (aType < Flush_InterruptibleLayout);
>       // mInFlush is a bitfield, so can't us AutoRestore here.  But we
>       // need to keep track of multi-level reentry correctly, so need
>       // to restore the old mInFlush value.
>       bool oldInFlush = mInFlush;
>       mInFlush = true;
>-      shell->FlushPendingNotifications(aType);
>+      if (shell->FlushPendingNotifications(aType)) {
>+        mNeedStyleFlush = false;
>+        mNeedLayoutFlush = mNeedLayoutFlush && (aType < Flush_InterruptibleLayout);
>+      }
>       mInFlush = oldInFlush;
>     }
>   }
> }

This code correctly handles only clearing mNeedLayoutFlush when aType requests flushing layout.

>     if (flushType >= (mSuppressInterruptibleReflows ? Flush_Layout : Flush_InterruptibleLayout) &&
>         !mIsDestroying) {
>+      isFlushed = true;

But this change looks like it means that in the normal codepath, you'll return false for Flush_Style, and thus not clear mNeedStyleFlush for repeated Flush_Style flushes.


This also breaks the fact that FlushPendingNotifications sometimes calls SetNeedLayoutFlush by moving the clearing of mNeedLayoutFlush to after the call to FlushPendingNotifications; the resulting code probably still works because you return false in that case, but it's extremely confusing.


It might be better to restructure this in line with the existing code in PresShell::FlushPendingNotifications, and instead of taking this approach, call mDocument->SetNeedLayoutFlush() and/or mDocument->SetNeedStyleFlush() where needed.  Though that might also have the problem of setting mNeedLayoutFlush/mNeedStyleFlush where they weren't set before.

Comment 5

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: layout_not_flush v2

Hi dbaron, roc
I updated patch base on dbaron's comment. How about this patch? Thanks

Comment 6

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

Comment on attachment 8446403 [details] [diff] [review]
layout_not_flush v2

>+  bool isStyleFlushed = false;
>+  bool isLayoutFlushed = false;

Please call these variables didStyleFlush and didLayoutFlush.




>+  if (!isStyleFlushed && flushType >= Flush_Style) {
>+    mDocument->SetNeedStyleFlush();
>+  }
>+
>+  if (!isLayoutFlushed &&
>+      (flushType >=
>+       (mSuppressInterruptibleReflows ? Flush_Layout : Flush_InterruptibleLayout))) {
>+    mDocument->SetNeedLayoutFlush();

Please add && !mIsDestroying to both of these tests.


Also please remove this existing chunk of code:

>    } else if (!mIsDestroying && mSuppressInterruptibleReflows &&
>               flushType == Flush_InterruptibleLayout) {
>      // We suppressed this flush, but the document thinks it doesn't
>      // need to flush anymore.  Let it know what's really going on.
>      mDocument->SetNeedLayoutFlush();

since your new code replaces it, but move the comment from this chunk to your new code.  Except you need to adjust the comment to reflect that suppressed could now mean either due to mSuppressInterruptibleReflows or isSafeToFlush (where previously it was only about mSuppressInterruptibleReflows).

r=dbaron with those changes

Comment 7

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

Comment on attachment 8446403 [details] [diff] [review]
layout_not_flush v2

>Bug 1029982 - Don't change mNeedLayoutFlush and mNeedStyleFlush when flush failed.

Also please adjust your commit message to reflect that it's no longer "don't change" but instead now setting them back to true.

Comment 8

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: layout_not_flush v3 (carries r+: dbaron, roc)

Update to address comment.

try push:  https://tbpl.mozilla.org/?tree=Try&rev=162015a63561

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ac07609c5d20

Comment 10

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

https://hg.mozilla.org/mozilla-central/rev/ac07609c5d20