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)
Tracking
()
RESOLVED
FIXED
mozilla57
Tracking | Status | |
---|---|---|
firefox-esr52 | --- | unaffected |
firefox55 | --- | wontfix |
firefox56 | --- | wontfix |
firefox57 | --- | fixed |
People
(Reporter: bc, Assigned: bzbarsky)
References
Details
(Keywords: assertion)
Attachments
(4 files)
25.17 KB,
text/plain
|
Details | |
599 bytes,
text/html
|
Details | |
11.03 KB,
patch
|
dholbert
:
review+
|
Details | Diff | Splinter Review |
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
Comment 1•7 years ago
|
||
The assertion was added in bug 1364320 landed in 55.
Flags: needinfo?(bzbarsky)
Assignee | ||
Comment 2•7 years ago
|
||
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.
Assignee | ||
Comment 3•7 years ago
|
||
Assignee | ||
Comment 4•7 years ago
|
||
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)
Comment 5•7 years ago
|
||
(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.
Assignee | ||
Comment 6•7 years ago
|
||
The assertion was added in bug 1364360.
Assignee | ||
Comment 7•7 years ago
|
||
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)
Assignee | ||
Comment 8•7 years ago
|
||
MozReview-Commit-ID: FU0Afemj4XD
Attachment #8908454 -
Flags: review?(dholbert)
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Assignee | ||
Comment 9•7 years ago
|
||
MozReview-Commit-ID: K54u9NmAlpn
Attachment #8908455 -
Flags: review?(dholbert)
Comment 10•7 years ago
|
||
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 11•7 years ago
|
||
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+
Assignee | ||
Comment 12•7 years ago
|
||
> 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.
Comment 13•7 years ago
|
||
(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.
Updated•7 years ago
|
Priority: -- → P1
Comment 14•7 years ago
|
||
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
Comment 15•7 years ago
|
||
bugherder |
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
Updated•7 years ago
|
status-firefox55:
--- → wontfix
status-firefox56:
--- → wontfix
status-firefox-esr52:
--- → unaffected
Assignee | ||
Comment 16•7 years ago
|
||
Fwiw, 55 and 56 are affected by the layout bug. They just don't have the assertion which catches it...
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•