1361268 - mCachedRootElement inadvertantly makes root element available even after it's been removed from child list

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, enhancement)

Description

[Daniel Holbert [:dholbert]]

When we do...
  document.removeChild(document.documentElement)
...it turns out that we have calls to nsIDocument GetRootElement() *during the removal* (e.g. inside of UpdateViewportScrollbarStylesOverride()).

Those GetRootElement() calls are likely to return the very root node that we're in the process of removing, because its value is still cached in mCachedRootElement!!!   In the example of UpdateViewportScrollbarStylesOverride(), this may confuse our scrollbar-styles-propagation code.  (I haven't been able to get it to make visible bad-effects like scrollbars incorrectly sticking around, but at the very least, it stymies the expectation that UpdateViewportScrollbarStylesOverride should be oblivious to elements that have already been removed from their parents' child lists.)

I think we should clear mCachedRootElement a bit sooner to address this.

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: Bug 1361268: Clear nsIDocument::mCachedRootElement *before* removing from doc's child list, rather than *after* removing.

Previously to this patch, we'd clear this pointer *after* doRemoveChildAt() --
but internally, doRemoveChildAt does quite a bit of cleanup, and it makes some
calls to nsIDocument::GetRootElement(), which will misleadingly return the
still-present mCachedRootElement member-var.  We'd really like those
invocations to find an empty cached variable, check the child list, and return
the actual new root element (probably null, since in this case we've just
removed the old root element).

Review commit: https://reviewboard.mozilla.org/r/135394/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/135394/

Comment 2

[Daniel Holbert [:dholbert]]

I think/hope it's as simple as just swapping these two lines (see attached MozReview patch), but I guess Try will help us find out. Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=bc4222f38d08

Note that nsINode::doRemoveChildAt basically removes from the child array *right away* (after which point, calls to GetRootElement will tell the truth about the post-removal document root).

If it were the case that doRemoveChildAt triggered a call to GetRootElement() *before* doing its removal from the child array, then this patch would be fruitless (because that call would just repopuplate the old mCachedRootElement value despite the fact that it's doomed to be removed).  But from my local testing, and from skimming the start of doRemoveChildAt, that doesn't seem to happen. Hooray!

Comment 3

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8863610 [details]
Bug 1361268: Clear nsIDocument::mCachedRootElement *before* removing from doc's child list, rather than *after* removing.

https://reviewboard.mozilla.org/r/135394/#review138626

r=me

::: dom/base/nsDocument.cpp:4180
(Diff revision 1)
>      // Destroy the link map up front before we mess with the child list.
>      DestroyElementMaps();
>    }
>  
> -  doRemoveChildAt(aIndex, aNotify, oldKid, mChildren);
>    mCachedRootElement = nullptr;

This is a case where I think most of the commit message should actually be in a code comment right above these two lines.

Also, maybe a comment in doRemoveChildAt about how it needs to make sure no code that could reset mCachedRootElement runs before it makes its aChildArray.RemoveChildAt call.

Comment 4

[Daniel Holbert [:dholbert]]

Comment on attachment 8863610 [details]
Bug 1361268: Clear nsIDocument::mCachedRootElement *before* removing from doc's child list, rather than *after* removing.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/135394/diff/1-2/

Comment 5

[Daniel Holbert [:dholbert]]

Comment on attachment 8863610 [details]
Bug 1361268: Clear nsIDocument::mCachedRootElement *before* removing from doc's child list, rather than *after* removing.

https://reviewboard.mozilla.org/r/135394/#review138626

> This is a case where I think most of the commit message should actually be in a code comment right above these two lines.
> 
> Also, maybe a comment in doRemoveChildAt about how it needs to make sure no code that could reset mCachedRootElement runs before it makes its aChildArray.RemoveChildAt call.

Good point. I've shortened the commit message a little, and I added explanatory comments in the places you suggested.

I also added an assert at the end of nsDocument::RemoveChildAt to be sure we didn't end up with the removed node in our root-element-cached-variable.  And I verified that I can make that assertion fire if I add an (unwanted) GetRootElement() call towards the beginning of doRemoveChildAt. Hooray!

Comment 6

[Daniel Holbert [:dholbert]]

Comment on attachment 8863610 [details]
Bug 1361268: Clear nsIDocument::mCachedRootElement *before* removing from doc's child list, rather than *after* removing.

https://reviewboard.mozilla.org/r/135394/#review138696

::: dom/base/nsDocument.cpp:4187
(Diff revision 2)
>    mCachedRootElement = nullptr;
> +  doRemoveChildAt(aIndex, aNotify, oldKid, mChildren);
> +  MOZ_ASSERT(mCachedRootElement != oldKid,
> +             "Stale pointer in mCachedRootElement, after we tried to clear it "
> +             "(maybe somebody called GetRootElement() too early?)");

(I wonder if we should only bother resetting mCachedRootElement if it *is* the removed node, oldKid.

i.e. maybe the nullptr assignment should really be:
  if (oldKid == mCachedRootElement) {
    mCachedRootElement = nullptr;
  } // else, assume mCachedRootElement is still correct

I assume it must not be as simple as that, though -- there are probably edge cases that make it a good idea to reset it whenever we remove a child.  (And it's not too expensive to recompute, as long as we're not doing RemoveChild(someNonRootNode)/GetRootElement back to back repeatedly.)

Comment 7

[Boris Zbarsky [:bzbarsky]]

> I also added an assert at the end of nsDocument::RemoveChildAt to be
> sure we didn't end up with the removed node in our root-element-cached-variable.

It's not clear to me how assertable that is, given things like XBL destructors and the arbitrary code they can run...

> (I wonder if we should only bother resetting mCachedRootElement if it *is* the removed node, oldKid.

I wouldn't worry about the complexity.  But yes, we could.

Comment 8

[Daniel Holbert [:dholbert]]

(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #7)
> > I also added an assert at the end of nsDocument::RemoveChildAt to be
> > sure we didn't end up with the removed node in our root-element-cached-variable.
> 
> It's not clear to me how assertable that is, given things like XBL
> destructors and the arbitrary code they can run...

It doesn't fail on a reftest/mochitest/crashtest/web-platform test try run, at least! So I'll plan on leaving it in for now, since it provides good enforcement of the expectations that would otherwise only be documented in comments. If it turns out it can be made to fail, we can remove it or make it more specific [as appropriate to the scenario that makes it fail]. 


> > (I wonder if we should only bother resetting mCachedRootElement if it *is* the removed node, oldKid.
> 
> I wouldn't worry about the complexity.  But yes, we could.

(I'm not going to bother with this idea, at this point.)

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0e22a71d67fd
Clear nsIDocument::mCachedRootElement *before* removing from doc's child list, rather than *after* removing. r=bz

Comment 10

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

https://hg.mozilla.org/mozilla-central/rev/0e22a71d67fd