Closed Bug 1398500 Opened 7 years ago Closed 7 years ago

Assertion failure: !OwnerDoc()->IsScrollingElement(this) (How can we have a scrollframe if we're the scrollingElement for our document?), at dom/base/Element.cpp:699

Categories

(Core :: DOM: Core & HTML, defect, P1)

Product:
Core
Component:
DOM: Core & HTML
Version:
56 Branch
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla57
Tracking Flags:
Tracking Status
firefox-esr52 --- unaffected
firefox55 --- wontfix
firefox56 --- wontfix
firefox57 --- fixed

People

(Reporter: bc, Assigned: bzbarsky)

References

Details

(Keywords: assertion)

Attachments

(4 files)

Linux assertion log with url scrubbed
25.17 KB, text/plain
Details
Testcase
599 bytes, text/html
Details
part 1. Switch the viewport scrollbar override stuff to use Element, not nsIContent
11.03 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
part 2. Make sure that if we start propagating scroll to viewport from a new body element we reframe it as needed
5.54 KB, patch
dholbert
: review+
Details | Diff | Splinter Review 
1. Load url (not provided due to possible pii). If you are working on this bug please ping me on irc or email.

2. Assertion failure: !OwnerDoc()->IsScrollingElement(this) (How can we have a scrollframe if we're the scrollingElement for our document?), at /mozilla/builds/nightly/mozilla/dom/base/Element.cpp:699

#01: mozilla::dom::Element::GetScrollFrame(nsIFrame**, mozilla::FlushType) (/mozilla/builds/nightly/mozilla/dom/base/Element.cpp:697 (discriminator 1))
#02: mozilla::dom::Element::GetClientAreaRect() (:?)
#03: NSAppUnitsToIntPixels(int, float) (/mozilla/builds/nightly/mozilla/firefox-debug/dist/include/nsCoord.h:399)
#04: mozilla::dom::GenericBindingGetter(JSContext*, unsigned int, JS::Value*) (/mozilla/builds/nightly/mozilla/dom/bindings/BindingUtils.cpp:2919)

Windows/Linux; Beta 56 and Nightly 57
The assertion was added in bug 1364320 landed in 55.
Flags: needinfo?(bzbarsky)
This assertion is actually catching a layout bug.  I'll attach a pretty minimal testcase, but briefly when we change which element's scrollbars are propagated to viewport we may need to do some reframing that we don't do.

The upshot is that we end up with incorrectly-visible scrollbars (in an opt build) and this assertion failure in a debug build.

Safari has the incorrectly-visible scrollbars too; Chrome doesn't seem to.
Attached file Testcase — 

    
    

    
  
But even if we fix that, there's still a problem: our definition of when scroll gets propagated to viewport considers scrollsnap stuff and smoothscrolling, not just overflow values.  This doesn't obviously match the CSS spec, and will mean this assertion could still fail if there's scroll-snap stuff or smoothscrolling on an overflow:visible root.

For example, this testcase:

  <!DOCTYPE html>
  <html>
    <head>
      <style>
        body { overflow: scroll; border: 1px solid green; }
        html { scroll-behavior: smooth; }
      </style>
    </head>
    <body>
      Should this have a body scrollbar?
    </body>
  </head>
  </html>

shows a scrollbar in Gecko but not in Chrome or Safari...  Botond, do you know why we do those checks in http://searchfox.org/mozilla-central/rev/6326724982c66aaeaf70bb7c7ee170f7a38ca226/layout/base/nsPresContext.cpp#1457-1465 (CheckOverflow)?
Flags: needinfo?(botond)

    
    

    
  
(In reply to Hsin-Yi Tsai [:hsinyi] from comment #1)
> The assertion was added in bug 1364320 landed in 55.

That does not seem like the right bug number.

    
    

    
  
The assertion was added in bug 1364360.

    
    

    
  
OK, I've spun off the question from comment 4 into bug 1400094, this time with a needinfo to hopefully the right person.
Flags: needinfo?(bzbarsky)
Flags: needinfo?(botond)

    
    

    
  


  
MozReview-Commit-ID: FU0Afemj4XD

        Attachment #8908454 -
        Flags: review?(dholbert)

    
  
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED

    
    

    
  
Comment on attachment 8908454 [details] [diff] [review]
part 1.  Switch the viewport scrollbar override stuff to use Element, not nsIContent

r=me, one nit:

>diff --git a/dom/base/Element.cpp b/dom/base/Element.cpp
>--- a/dom/base/Element.cpp
>+++ b/dom/base/Element.cpp
>       if (presContext) {
>         MOZ_ASSERT(this !=
>-                   presContext->GetViewportScrollbarStylesOverrideNode(),
>+                   presContext->GetViewportScrollbarStylesOverrideElement(),
>                    "Leaving behind a raw pointer to this node (as having "
>                    "propagated scrollbar styles) - that's dangerous...");

s/node/element/ in the assertion message here, probably.

        Attachment #8908454 -
        Flags: review?(dholbert) → review+

    
    

    
  
Comment on attachment 8908455 [details] [diff] [review]
part 2.  Make sure that if we start propagating scroll to viewport from a new body element we reframe it as needed

Review of attachment 8908455 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with nits:

::: layout/base/crashtests/1398500.html
@@ +17,5 @@
> +  </head>
> +  <body>
> +    First body
> +  </body>
> +</head>

This </head> (after the </body>) is redundant (the head was already closed, before the body opened).

This affects both of the reftest HTML files, too (along with this crashtest HTML file).

::: layout/base/nsCSSFrameConstructor.cpp
@@ +8619,4 @@
>      // We might be removing the element that we propagated viewport scrollbar
>      // styles from.  Recompute those. (This clause covers two of the three
>      // possible scrollbar-propagation sources: the <body> [as aChild or a
>      // descendant] and the root node. The other possible scrollbar-propagation

This comment (which splinter probably won't quote sufficiently) looks like it to be removed/updated. In particular:
 * s/We might be removing/We are removing/ (right?)
 * The bit about "This clause covers [...] the <body> and the root node" -- that part seems to be discussing the old "if" condition, which you're removing in this patch.  I think now we'll be covering *all* possible sources of scrollbar propagation, not just two out of the three... which makes the comment incorrect. Right?

@@ +8626,5 @@
> +    Element* newOverrideElement =
> +      presContext->UpdateViewportScrollbarStylesOverride();
> +
> +    // Now if newOverrideElement is not the root and isn't aChild (which can
> +    // happen if all we're doing here is reframing the current override

s/which can happen/which it could be/

I think, in that parenthetical, you're meaning to explain a scenario underwhich newOverrideElement would be equal to aChild, right?  Hopefully this ^^ rewording makes that clearer.  Right now, the logic of the sentence seems to suggest the *opposite* -- the phrasing "if newOverrideElement...isn't aChild (which can happen...)" sounds like you're saying that that a "newOverrideElement isn't aChild scenario *can happen*".   (And of course that scenario *can* indeed happen, but that's not the scenario you're meaning to highlight in the parenthetical.)

        Attachment #8908455 -
        Flags: review?(dholbert) → review+

    
    

    
  
> s/node/element/ in the assertion message here, probably.

Will fix.

> This </head> (after the </body>) is redundant (the head was already closed, before the body opened).

Will fix, in all files.

> * s/We might be removing/We are removing/ (right?)

No, see below.

> I think now we'll be covering *all* possible sources of scrollbar propagation, not just two out of the
> three... which makes the comment incorrect. Right?

No, because the fullscreen case does not set mViewportScrollbarOverrideNode...

> s/which can happen/which it could be/

Good idea, will do.

    
    

    
  
(In reply to Boris Zbarsky [:bz] (still digging out from vacation mail) from comment #12)
> No, see below.
[...]
> No, because the fullscreen case does not set
> mViewportScrollbarOverrideNode...

Ah, ok! Thanks. I guess that comment is still correct then, including the part about depending on code elsewhere for fullscreen handling.
Priority: -- → P1

    
    

    
  
Pushed by bzbarsky@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4553f8ff4114
part 1.  Switch the viewport scrollbar override stuff to use Element, not nsIContent.  r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/28bdfd821210
part 2.  Make sure that if we start propagating scroll to viewport from a new body element we reframe it as needed.  r=dholbert

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/4553f8ff4114
https://hg.mozilla.org/mozilla-central/rev/28bdfd821210
Status: ASSIGNED → RESOLVED
Closed: 7 years ago

          status-firefox57:
          --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57

    
    

    
  
Fwiw, 55 and 56 are affected by the layout bug.  They just don't have the assertion which catches it...

    
  
Depends on: 1400599

    
  
Blocks: 1364360
Component: DOM → DOM: Core & HTML

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: