Bug 1935728 Comment 9 Edit History

Note: The actual edited comment in the bug view page will always show the original commenter’s name and original timestamp.

Original comment by
on 
So in that pernosco session, we get two important calls to `nsCSSFrameConstructor::ContentWillBeRemoved`:

(1) We get a call to `nsCSSFrameConstructor::ContentWillBeRemoved` where `aChild` is our `HTMLBodyElement`.  At this point `nsPresContext::mViewportScrollOverrideElement` is pointing to that body element, and we successfully null out that pointer in `UpdateViewportScrollStylesOverride`, because its call to `GetPropagatedScrollStylesForViewport` explicitly passes down the removed-child-pointer when calling `GetBodyElement`, and so `GetBodyElement` is smart enough to return null in that case: 
https://searchfox.org/mozilla-central/rev/5ad94327fc03d0e5bc6aa81c0d7d113c2dccf947/layout/base/nsPresContext.cpp#1608,1639-1642
```cpp
static Element* GetPropagatedScrollStylesForViewport(
...
  Element* bodyElement = document->GetBodyElement(aRemovedChild);
  if (!bodyElement) {
    return nullptr;
  }
```
(2) Then we get a call to `nsCSSFrameConstructor::ContentWillBeRemoved` where `aChild` is an `<html>` element -- it's of type `mozilla::dom::HTMLSharedElement` but its `aChild->NodeInfo()->mLocalName->mData` is the string `"html"`.  This time, this ContentWillBeRemoved call has flags `aFlags=REMOVE_FOR_RECONSTRUCTION` (not `REMOVE_CONTENT`) which means `removingElement` ends up nullptr here:
https://searchfox.org/mozilla-central/rev/5ad94327fc03d0e5bc6aa81c0d7d113c2dccf947/layout/base/nsCSSFrameConstructor.cpp#7334-7337
```cpp
const Element* removingElement =
    aFlags == REMOVE_CONTENT ? aChild->AsElement() : nullptr;
Element* newOverrideElement =
    presContext->UpdateViewportScrollStylesOverride(removingElement);
```
So, inside of `UpdateViewportScrollStylesOverride`, we don't have any prohibitions on matched elements, and we **restore `mViewportScrollOverrideElement` to be pointing at the `body` element that we're in the process of removing**.
Revision 1 by
on 
So in that pernosco session, we get two important calls to `nsCSSFrameConstructor::ContentWillBeRemoved`, just before the assertion failure:

(1) We get a call to `nsCSSFrameConstructor::ContentWillBeRemoved` where `aChild` is our `HTMLBodyElement`.  At this point `nsPresContext::mViewportScrollOverrideElement` is pointing to that body element, and we successfully null out that pointer in `UpdateViewportScrollStylesOverride`, because its call to `GetPropagatedScrollStylesForViewport` explicitly passes down the removed-child-pointer when calling `GetBodyElement`, and so `GetBodyElement` is smart enough to return null in that case: 
https://searchfox.org/mozilla-central/rev/5ad94327fc03d0e5bc6aa81c0d7d113c2dccf947/layout/base/nsPresContext.cpp#1608,1639-1642
```cpp
static Element* GetPropagatedScrollStylesForViewport(
...
  Element* bodyElement = document->GetBodyElement(aRemovedChild);
  if (!bodyElement) {
    return nullptr;
  }
```
(2) Then we get a call to `nsCSSFrameConstructor::ContentWillBeRemoved` where `aChild` is an `<html>` element -- it's of type `mozilla::dom::HTMLSharedElement` but its `aChild->NodeInfo()->mLocalName->mData` is the string `"html"`.  This time, this ContentWillBeRemoved call has flags `aFlags=REMOVE_FOR_RECONSTRUCTION` (not `REMOVE_CONTENT`) which means `removingElement` ends up nullptr here:
https://searchfox.org/mozilla-central/rev/5ad94327fc03d0e5bc6aa81c0d7d113c2dccf947/layout/base/nsCSSFrameConstructor.cpp#7334-7337
```cpp
const Element* removingElement =
    aFlags == REMOVE_CONTENT ? aChild->AsElement() : nullptr;
Element* newOverrideElement =
    presContext->UpdateViewportScrollStylesOverride(removingElement);
```
So, inside of `UpdateViewportScrollStylesOverride`, we don't have any prohibitions on matched elements, and we **restore `mViewportScrollOverrideElement` to be pointing at the `body` element that we're in the process of removing**.
Revision 2 by
on 
So in that pernosco session, we get two important calls to `nsCSSFrameConstructor::ContentWillBeRemoved`, just before the assertion failure:

(1) We get a call to `nsCSSFrameConstructor::ContentWillBeRemoved` where `aChild` is our `HTMLBodyElement`.  At this point `nsPresContext::mViewportScrollOverrideElement` is pointing to that body element, and we successfully null out that pointer in `UpdateViewportScrollStylesOverride`, because its call to `GetPropagatedScrollStylesForViewport` explicitly passes down the removed-child-pointer when calling `GetBodyElement`, and so `GetBodyElement` is smart enough to return null in that case: 
https://searchfox.org/mozilla-central/rev/5ad94327fc03d0e5bc6aa81c0d7d113c2dccf947/layout/base/nsPresContext.cpp#1608,1639-1642
```cpp
static Element* GetPropagatedScrollStylesForViewport(
...
  Element* bodyElement = document->GetBodyElement(aRemovedChild);
  if (!bodyElement) {
    return nullptr;
  }
```
This is all good and it's doing the cleanup that the comment that I quoted in comment 8 was alluding to.


(2) Then we get a call to `nsCSSFrameConstructor::ContentWillBeRemoved` where `aChild` is an `<html>` element -- it's of type `mozilla::dom::HTMLSharedElement` but its `aChild->NodeInfo()->mLocalName->mData` is the string `"html"`.  This time, this ContentWillBeRemoved call has flags `aFlags=REMOVE_FOR_RECONSTRUCTION` (not `REMOVE_CONTENT`) which means `removingElement` ends up nullptr here:
https://searchfox.org/mozilla-central/rev/5ad94327fc03d0e5bc6aa81c0d7d113c2dccf947/layout/base/nsCSSFrameConstructor.cpp#7334-7337
```cpp
const Element* removingElement =
    aFlags == REMOVE_CONTENT ? aChild->AsElement() : nullptr;
Element* newOverrideElement =
    presContext->UpdateViewportScrollStylesOverride(removingElement);
```
So, inside of `UpdateViewportScrollStylesOverride`, we don't have any prohibitions on matched elements, and we **restore `mViewportScrollOverrideElement` to be pointing at the `body` element that we're in the process of removing**.
Revision 3 by
on 
So in that pernosco session, we get two important calls to `nsCSSFrameConstructor::ContentWillBeRemoved`, just before the assertion failure:

(1) We get a call to `nsCSSFrameConstructor::ContentWillBeRemoved` where `aChild` is our `HTMLBodyElement`.  At this point `nsPresContext::mViewportScrollOverrideElement` is pointing to that body element, and we successfully null out that pointer in `UpdateViewportScrollStylesOverride`, because its call to `GetPropagatedScrollStylesForViewport` explicitly passes down the removed-child-pointer when calling `GetBodyElement`, and so `GetBodyElement` is smart enough to return null in that case: 
https://searchfox.org/mozilla-central/rev/5ad94327fc03d0e5bc6aa81c0d7d113c2dccf947/layout/base/nsPresContext.cpp#1608,1639-1642
```cpp
static Element* GetPropagatedScrollStylesForViewport(
...
  Element* bodyElement = document->GetBodyElement(aRemovedChild);
  if (!bodyElement) {
    return nullptr;
  }
```
This is all good and it's doing the cleanup that the comment that I quoted in comment 8 was alluding to.


(2) Then we get a call to `nsCSSFrameConstructor::ContentWillBeRemoved` where `aChild` is an `<html>` element  (in C++ it's of type `mozilla::dom::HTMLSharedElement` but its `aChild->NodeInfo()->mLocalName->mData` is the string `"html"`).  This time, this ContentWillBeRemoved call has flags `aFlags=REMOVE_FOR_RECONSTRUCTION` (not `REMOVE_CONTENT`) which means `removingElement` ends up nullptr here:
https://searchfox.org/mozilla-central/rev/5ad94327fc03d0e5bc6aa81c0d7d113c2dccf947/layout/base/nsCSSFrameConstructor.cpp#7334-7337
```cpp
const Element* removingElement =
    aFlags == REMOVE_CONTENT ? aChild->AsElement() : nullptr;
Element* newOverrideElement =
    presContext->UpdateViewportScrollStylesOverride(removingElement);
```
So, inside of `UpdateViewportScrollStylesOverride`, we don't have any prohibitions on matched elements, and we **restore `mViewportScrollOverrideElement` to be pointing at the `body` element that we're in the process of removing**.

Back to Bug 1935728 Comment 9