897763 - "ASSERTION: Shouldn't be trying to restyle non-elements directly" when starting a drag operation on about:newtab

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Tim Taubert [:ttaubert] (inactive)]

Attachment: backtrace of the assertion

STR:

0) Have a debug build
1) Open a new tab with about:newtab
2) Start dragging one of the sites shown on about:newtab to another position

Result:

Lots of assertions.


AFAICT, nsEventStateManager::GenerateDragGesture() is called and dispatches a dragstart event. The about:newtab page calls .getBoundingClientRect() on the dragged DOMElement which causes a RestyleManager::ProcessPendingRestyles() call that then leads to the given assertion.

Comment 1

[Tim Taubert [:ttaubert] (inactive)]

CC'ing a couple of people that worked on similar bugs.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Cameron, want to take a look?

Comment 3

[Cameron McCormack (:heycam)]

Sure.

Comment 4

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

I expect the bulk of these assertions to be fixed by bug 828312 and related work.

Comment 5

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

Actually, maybe bug 898333 only fixes such assertions when they're regressions from the other work in bug 828312.

In any case, this sort of assertion is most likely to be caused by the concept the code refers to as hints not handled for descendents, hints not handled by parent, or nonInheritedHints (though bug 898209 eliminates the third term in favor of the second).  See:
http://mxr.mozilla.org/mozilla-central/search?string=nothandledfordescendants

In particular, it could be caused by:

 * overly pessimistic assumptions about which hints aren't handled (which bug 828312 is attempting to fix some of, I think... though maybe, again, that's only for cases that it itself regresses; though I know I have some patches in my patch queue that attempt to make such fixes in other cases)

 * cases where we return a hint not handled for descendants for an inherited property.  This is always a mistake.  It's both extremely inefficient and leads to the exact situation this assertion describes.  (See next paragraph for explanation.)


The underlying concept here is that style difference calculation (nsStyleContext::CalcStyleDifference, nsStyle*::CalcDifference, ElementRestyler::CaptureChange) involves comparing new and old styles and returning a bitmask (nsChangeHint) of what needs to change to cause the updates required by that style change.  Most of these hints apply to an entire subtree; if they're returned for an element, it is unnecessary to process the same hint for a descendant.  However, some of these hints (see mxr search above and code in nsChangeHint.h) don't imply that, and need to be returned for every frame that has that style change, even if an ancestor had the same change.

Comment 6

[Cameron McCormack (:heycam)]

The assertions started after bug 875037 landed.

Comment 7

[Cameron McCormack (:heycam)]

Attachment: test

Here's a reduced test that triggers the assertion.  It's a change to pointer-events that is causing it.

Comment 8

[Cameron McCormack (:heycam)]

What's happening is that the pointer-events change is inherited (it's on nsStyleVisibility), and the frame structure is like this:

Canvas(html) [state=000b002000000200] :-moz-scrolled-canvas <
  Block(html) [state=000b100000d00200] <
    line <
      Block(body) [state=000b120000100200] <
        line <
          Placeholder(div) [state=0000000000200000] :-moz-non-element outOfFlowFrame=Block(div)@0x115057018
        >
      >
    >
  >
>
AbsoluteList <
  Block(div)@0x115057018 [state=0002162000d00300] <
    line <
      Placeholder(div) [state=0000000000200000] :-moz-non-element outOfFlowFrame=HTMLScroll(div)@0x1150573a8
    >
    AbsoluteList <
      HTMLScroll(div)@0x1150573a8 [state=0002060000000100] <
        Block(div) [state=0002102000d00200] :-moz-scrolled-content <
          line <
            Text(0)"x" [state=00010000b0600000] :-moz-non-element
          >
        >
      >
    >
  >
>

When we get to restyling the text frame, we use the scroll frame as the provider for the parent, and so get into here:

https://hg.mozilla.org/mozilla-central/file/tip/layout/base/RestyleManager.cpp#l2128

and then CalcStyleDifference will return nsChangeHint_NeedReflow due to the pointer-events change and that bit being in aParentHintsNotHandledForDescendants.

Comment 9

[Cameron McCormack (:heycam)]

David, is the issue here how we represent the reflow and dirty reflow hints?  The fact the (nsChangeHint_NeedReflow) is a "hint not handled by parent", while (nsChangeHint_NeedReflow | nsChangeHint_NeedDirtyReflow) is handled by the parent, seems to be problematic.

When nsStyleVisibility::CalcDifference returns (nsChangeHint_NeedReflow | nsChangeHint_NeedDirtyReflow), it is indicating a change that is handled by the parent.

But the (maxDifference & aParentHintsNotHandledForDescendants) check in DO_STRUCT_DIFFERENCE, which lets us skip calling CalcDifference on a given style struct if it's not going to return any hints that are not handled by the parent, is basically saying "well, nsStyleVisibility::CalcDifference might return just nsChangeHint_NeedReflow, a hint not handled by the parent, so I'd better call it and add its result to |hint|".  This is even though we'll only ever return it with nsChangeHint_NeedDirtyReflow, converting it into a hint that will be handled by the parent, and so we wouldn't actually ever be returning a hint not handled by the parent.

Comment 10

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

(In reply to Cameron McCormack (:heycam) from comment #9)
> David, is the issue here how we represent the reflow and dirty reflow hints?
> The fact the (nsChangeHint_NeedReflow) is a "hint not handled by parent",
> while (nsChangeHint_NeedReflow | nsChangeHint_NeedDirtyReflow) is handled by
> the parent, seems to be problematic.

Yes, it's a pain to deal with, but it's also a rather important optimization.

> When nsStyleVisibility::CalcDifference returns (nsChangeHint_NeedReflow |
> nsChangeHint_NeedDirtyReflow), it is indicating a change that is handled by
> the parent.
> 
> But the (maxDifference & aParentHintsNotHandledForDescendants) check in
> DO_STRUCT_DIFFERENCE, which lets us skip calling CalcDifference on a given
> style struct if it's not going to return any hints that are not handled by
> the parent, is basically saying "well, nsStyleVisibility::CalcDifference
> might return just nsChangeHint_NeedReflow, a hint not handled by the parent,
> so I'd better call it and add its result to |hint|".  This is even though
> we'll only ever return it with nsChangeHint_NeedDirtyReflow, converting it
> into a hint that will be handled by the parent, and so we wouldn't actually
> ever be returning a hint not handled by the parent.

So aParentHintsNotHandledForDescendants should have the hint removed if it's NeedReflow|NeedDirtyReflow, but it might contain a NeedReflow hint alone, since it should have been calculated using NS_HintsNotHandledForDescendantsIn.  So I think the problem here is that aParentHintsNotHandledForDescendants is wrong.  The question is where it came from -- possibly from one of the cases where RestyleSelf is overly pessimistic, some of which might be fixed by changes I'm working on.

(As I said in comment 5 one of the invariants of these hints is that something that's *inherited* should never retun a hint that's not handled by descendants.  nsStyleVisibility::CalcDifference seems to honor that rule, since a pointer-events change returns NeedReflow|NeedDirtyReflow.)

Comment 11

[Cameron McCormack (:heycam)]

(In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from comment #10)
> (In reply to Cameron McCormack (:heycam) from comment #9)
> > David, is the issue here how we represent the reflow and dirty reflow hints?
> > The fact the (nsChangeHint_NeedReflow) is a "hint not handled by parent",
> > while (nsChangeHint_NeedReflow | nsChangeHint_NeedDirtyReflow) is handled by
> > the parent, seems to be problematic.
> 
> Yes, it's a pain to deal with, but it's also a rather important optimization.

Can you explain how this optimization works, and why having two independent reflow hints wouldn't work?

> > When nsStyleVisibility::CalcDifference returns (nsChangeHint_NeedReflow |
> > nsChangeHint_NeedDirtyReflow), it is indicating a change that is handled by
> > the parent.
> > 
> > But the (maxDifference & aParentHintsNotHandledForDescendants) check in
> > DO_STRUCT_DIFFERENCE, which lets us skip calling CalcDifference on a given
> > style struct if it's not going to return any hints that are not handled by
> > the parent, is basically saying "well, nsStyleVisibility::CalcDifference
> > might return just nsChangeHint_NeedReflow, a hint not handled by the parent,
> > so I'd better call it and add its result to |hint|".  This is even though
> > we'll only ever return it with nsChangeHint_NeedDirtyReflow, converting it
> > into a hint that will be handled by the parent, and so we wouldn't actually
> > ever be returning a hint not handled by the parent.
> 
> So aParentHintsNotHandledForDescendants should have the hint removed if it's
> NeedReflow|NeedDirtyReflow, but it might contain a NeedReflow hint alone,
> since it should have been calculated using
> NS_HintsNotHandledForDescendantsIn.  So I think the problem here is that
> aParentHintsNotHandledForDescendants is wrong.  The question is where it
> came from -- possibly from one of the cases where RestyleSelf is overly
> pessimistic, some of which might be fixed by changes I'm working on.

That's right; I meant to point that out in comment 8 but I guess I shouldn't have used an hg.mozilla.org tip URL.  This is where we get to:

https://hg.mozilla.org/mozilla-central/file/d0edf8086809/layout/base/RestyleManager.cpp#l2134

So it's one of the pessimistic cases where we pass in all possible hints not handled by the parent.  Is that a case that will be fixed in your changes?  Is there a way we can know not to include the NeedReflow hint there?

Comment 12

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

(In reply to Cameron McCormack (:heycam) from comment #11)
> (In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from
> comment #10)
> > (In reply to Cameron McCormack (:heycam) from comment #9)
> > > David, is the issue here how we represent the reflow and dirty reflow hints?
> > > The fact the (nsChangeHint_NeedReflow) is a "hint not handled by parent",
> > > while (nsChangeHint_NeedReflow | nsChangeHint_NeedDirtyReflow) is handled by
> > > the parent, seems to be problematic.
> > 
> > Yes, it's a pain to deal with, but it's also a rather important optimization.
> 
> Can you explain how this optimization works, and why having two independent
> reflow hints wouldn't work?

Two independent hints would be fine, but I think it would just be equivalent and not actually fix anything.

To see how it works -- essentially, read RestyleManager::StyleChangeReflow and PresShell::FrameNeedsReflow.  Anything that requires a reflow needs to set NeedReflow, but there are three other bits that can be set along with it (but are invalid without it).

The idea is that if a property like 'width' is changed, we don't force reflow of all descendants -- we allow them to optimize away resizes using the normal resizing optimizations.  This makes dynamic changes of such properties much faster.

> That's right; I meant to point that out in comment 8 but I guess I shouldn't
> have used an hg.mozilla.org tip URL.  This is where we get to:
> 
> https://hg.mozilla.org/mozilla-central/file/d0edf8086809/layout/base/
> RestyleManager.cpp#l2134
> 
> So it's one of the pessimistic cases where we pass in all possible hints not
> handled by the parent.  Is that a case that will be fixed in your changes? 
> Is there a way we can know not to include the NeedReflow hint there?

Sorry, I missed that.

That should be fixed by this patch stack:
https://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/2d945162248c/pass-noninheritedhints-back
https://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/2d945162248c/better-noninheritedhints
which really ought to have its own bug, since it turned out not to be related to the bug it's currently marked as.  Maybe this bug, actually.

I should do a little more thinking about whether that stack is really correct, though.  It depends on whether the not-handled-for-descendants bits are about frame-tree-descendants or content-tree-descendants.

Comment 13

[Cameron McCormack (:heycam)]

(In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from comment #12)
> Two independent hints would be fine, but I think it would just be equivalent
> and not actually fix anything.

I was thinking that by having the hints be independent, we could stop returning nsChangeHint_NeedReflow from nsStyleVisibility::CalcDifference and also nsStyleVisibility::MaxDifference.  Since nsChangeHint_NeedDirtyReflow would still not be in nsChangeHint_Hints_NotHandledForDescendants, and thus wouldn't be set in the pessimistic case we're hitting, we'd then avoid propagating hints down to the text frame.

But maybe this is unnecessary with your patches.

Comment 14

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

(In reply to Cameron McCormack (:heycam) from comment #13)
> I was thinking that by having the hints be independent, we could stop
> returning nsChangeHint_NeedReflow from nsStyleVisibility::CalcDifference and
> also nsStyleVisibility::MaxDifference.  Since nsChangeHint_NeedDirtyReflow
> would still not be in nsChangeHint_Hints_NotHandledForDescendants, and thus
> wouldn't be set in the pessimistic case we're hitting, we'd then avoid
> propagating hints down to the text frame.

Ah, indeed.  Though a bit ugly since it would depend on an optimization for not asserting.  Then again, it would also make that optimization stronger.  So maybe worth doing.

Comment 15

[Mats Palmgren (inactive)]

This assertion is quite common in mochitest-browser-chrome (and I assume other mochitests too)
and given that generating the stack for each of these is quite time consuming and increases
the log size, perhaps we can make it a NS_WARNING for now until this bug is fixed?

Comment 16

[Cameron McCormack (:heycam)]

Attachment: restyle-text-warning

Yes let's do that.  This knocks the uncompressed log size on Linux x64 down from 40M to 36M.

Comment 17

[Cameron McCormack (:heycam)]

Attachment: patch

David, how does this look to avoid the problem?  This solves the specific pointer-events issue in about:newtab, and also means there are no more instances of this assertion in mochitest-browser-chrome runs.

Instead of making nsChangeHint_NeedReflow and nsChangeHint_NeedDirtyReflow completely independent bits, this patch adds a function on to style structs that can report whether the nsChangeHint_NeedReflow and nsChangeHint_ClearAncestorIntrinsics hints can ever be returned from CalcDifference as inherited hints.  This allows the optimisation in nsStyleContext::CalcStyleDifference to "correctly" skip comparing style structs in the pessimistic case mentioned in comment 11.

Alternatively, if your patch queue can solve this in a better way, we can not take this patch.

Green try run: https://tbpl.mozilla.org/?tree=Try&rev=6dcf6bb72d85

Comment 18

[Mats Palmgren (inactive)]

Comment on attachment 805149 [details] [diff] [review]
restyle-text-warning

I see that David is already involved here so he might as well review this
part too.

Comment 19

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

Comment on attachment 805833 [details] [diff] [review]
patch

I'd rather the nsStyleStructDifferenceAssertions assertions not
be in a static constructor.  How about a debug-only function called 
either from the module constructor or from some class's constructor?

The SVG and SVGReset and Visibility structs should have an "// XXX 
remove me: bug 876085" comment (for NeedReflow only) to match the
comments in the CalcDifference methods.

r=dbaron, and sorry for the delay

Comment 20

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

Comment on attachment 805149 [details] [diff] [review]
restyle-text-warning

Is this still needed?  (Also, better to change NS_ASSERTION to NS_WARN_IF_FALSE than add #ifdefs etc.)

Comment 21

[Cameron McCormack (:heycam)]

Comment on attachment 805149 [details] [diff] [review]
restyle-text-warning

No we don't need this one if we take the other patch.

Comment 22

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

One other thought, though:  maybe in the long run we want to teach the caller of CalcStyleDifference to throw away hints that were already handled by an ancestor.  Seems like it might speed some cases up, too.

Comment 23

[Cameron McCormack (:heycam)]

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

Comment 24

[Carsten Book [:Tomcat]]

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