1387243 - Debug assertion hit in nsDocument::UnblockDOMContentLoaded when stopping loading of timeout content from add-ons

Status: RESOLVED FIXED

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

Description

[Brad Werth [:bradwerth]]

Steps to reproduce on a Stylo OS X debug build (though issue doesn't appear to be specific to Stylo):

1) Install WebExtension version of Ad Block Plus from https://downloads.adblockplus.org/devbuilds/adblockplusfirefox/.
2) Launch Firefox, wait for all chrome content to load, then move cursor over the ABP icon in toolbar.

This appears to be a situation where a parser has been created for a document, but content is never loaded and so the parsing never starts. In that case, nsDocument::BeginLoad() is never called. When the load is terminated at http://searchfox.org/mozilla-central/source/docshell/base/nsDocShell.cpp#10703, that eventually calls nsDocument::EndLoad(), which leads to a debug assertion http://searchfox.org/mozilla-central/source/dom/base/nsDocument.cpp#5396.

This is very likely the cause of the intermittent oranges in Bug 1299022.

Comment 1

[Brad Werth [:bradwerth]]

Attachment: Bug 1387243 Part 1: Split nsDocument::EndLoad into a part that runs unconditionally, and a part that runs only when there is a matching BeginLoad call.

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

Comment 2

[Brad Werth [:bradwerth]]

Comment on attachment 8893579 [details]
Bug 1387243 Part 1: Split nsDocument::EndLoad into a part that runs unconditionally, and a part that runs only when there is a matching BeginLoad call.

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

Comment 3

[Brad Werth [:bradwerth]]

Comment on attachment 8893579 [details]
Bug 1387243 Part 1: Split nsDocument::EndLoad into a part that runs unconditionally, and a part that runs only when there is a matching BeginLoad call.

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

Comment 4

[Brad Werth [:bradwerth]]

Comment on attachment 8893579 [details]
Bug 1387243 Part 1: Split nsDocument::EndLoad into a part that runs unconditionally, and a part that runs only when there is a matching BeginLoad call.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/164652/diff/3-4/

Comment 5

[Brad Werth [:bradwerth]]

Comment on attachment 8893579 [details]
Bug 1387243 Part 1: Split nsDocument::EndLoad into a part that runs unconditionally, and a part that runs only when there is a matching BeginLoad call.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/164652/diff/4-5/

Comment 6

[Boris Zbarsky [:bzbarsky]]

> When the load is terminated at ...

Those links are already stale.  :(  Please use permalinks...

Is it the mBlockDOMContentLoaded assert in nsDocument::UnblockDOMContentLoaded that fails?

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8893579 [details]
Bug 1387243 Part 1: Split nsDocument::EndLoad into a part that runs unconditionally, and a part that runs only when there is a matching BeginLoad call.

https://reviewboard.mozilla.org/r/164652/#review171048

::: dom/base/nsDocument.cpp:5411
(Diff revision 5)
>  nsDocument::EndLoad()
>  {
> +  // Handle cases like timeouts where we might call this function without
> +  // ever having called BeginLoad.
> +  if (!mDidCallBeginLoad) {
> +    return;

Are we sure its safe to not do the EndLoad observer notifications?  Have you checked the existing EndLoad observer implementations?

::: dom/base/nsDocument.cpp:5426
(Diff revision 5)
>  
>    NS_DOCUMENT_NOTIFY_OBSERVERS(EndLoad, (this));
>  
>    UnblockDOMContentLoaded();
> +
> +  mDidCallBeginLoad = false;

Should this be before UnblockDOMContentLoaded, and indeed before anything else in this function other than the early return?  I worry about what happens if script runs before we reset this boolean.

::: dom/base/nsIDocument.h:3292
(Diff revision 5)
>  
>    // Whether <style scoped> support is enabled in this document.
>    enum { eScopedStyle_Unknown, eScopedStyle_Disabled, eScopedStyle_Enabled };
>    unsigned int mIsScopedStyleEnabled : 2;
>  
> +  bool mDidCallBeginLoad : 1;

Please put this with the other boolean bits; I have zero confidence in our compilers packing booleans bits with unsigned int bits.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Please see questions in comment 7?

Comment 9

[Brad Werth [:bradwerth]]

(In reply to Boris Zbarsky [:bz] (vacation Aug 14-27) from comment #6)
> > When the load is terminated at ...
> 
> Those links are already stale.  :(  Please use permalinks...
> 
> Is it the mBlockDOMContentLoaded assert in
> nsDocument::UnblockDOMContentLoaded that fails?

Yes. I'm sorry about the mixup with the links. I would appreciate info on how to get permalinks from searchfox.org results.

Comment 10

[Brad Werth [:bradwerth]]

Comment on attachment 8893579 [details]
Bug 1387243 Part 1: Split nsDocument::EndLoad into a part that runs unconditionally, and a part that runs only when there is a matching BeginLoad call.

https://reviewboard.mozilla.org/r/164652/#review171048

> Are we sure its safe to not do the EndLoad observer notifications?  Have you checked the existing EndLoad observer implementations?

[http://searchfox.org/mozilla-central/search?q=EndLoad(nsIDocument](http://searchfox.org/mozilla-central/search?q=EndLoad(nsIDocument) shows two observers...

1. layout/base/PresShell.cpp has an observer to call RestoreRootScrollPosition(). I don't think there are serious implications if we fail to call that method in a failed-to-parse case.
2. image/VectorImage.cpp has an observer to call OnSVGDocumentParsed(), which potentially calls OnSVGDocumentError(). The code comments indicate that the error is thrown precisely for the failed-to-parse case, so this is likely problematic. The proposed approach in this patch would prevent that error from being thrown.

Upon reflection, it's also important to set mParser = nullptr before exiting, or we'll leak. I guess this case of calling EndLoad without ever calling BeginLoad is somewhat planned for in both the observers and in the EndLoad call itself -- it's just UnblockDOMContentLoaded that doesn't account for the possibily and so it has an overzealous debug assert.

I'm going to rework the patch to instead clarify the logic and comments in UnblockDOMContentLoaded, since all the other moving parts seem to handle this case gracefully enough.

Comment 11

[Boris Zbarsky [:bzbarsky]]

> I would appreciate info on how to get permalinks from searchfox.org results.

There's a "Permalink" option in the "Navigation" menu at the top right.  Just click that, or copy the link url.

The plan from comment 10 sounds good.  Thank you!

Comment 12

[Brad Werth [:bradwerth]]

Comment on attachment 8893579 [details]
Bug 1387243 Part 1: Split nsDocument::EndLoad into a part that runs unconditionally, and a part that runs only when there is a matching BeginLoad call.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/164652/diff/5-6/

Comment 13

[Brad Werth [:bradwerth]]

(In reply to Brad Werth [:bradwerth] from comment #10)
> I'm going to rework the patch to instead clarify the logic and comments in
> UnblockDOMContentLoaded, since all the other moving parts seem to handle
> this case gracefully enough.

Since UnblockDOMContentLoaded is also called from outside of EndLoad, I didn't want to change its internal logic to detect whether or not BeginLoad had been called. Instead I changed EndLoad to split that function into an unconditional part, and a part that only runs when there has been a BeginLoad call. The call to UnblockDOMContentLoaded is in the second, conditional part.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8893579 [details]
Bug 1387243 Part 1: Split nsDocument::EndLoad into a part that runs unconditionally, and a part that runs only when there is a matching BeginLoad call.

https://reviewboard.mozilla.org/r/164652/#review172494

r=me if you document the new member.

Comment 15

[Brad Werth [:bradwerth]]

Comment on attachment 8893579 [details]
Bug 1387243 Part 1: Split nsDocument::EndLoad into a part that runs unconditionally, and a part that runs only when there is a matching BeginLoad call.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/164652/diff/6-7/

Comment 16

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8893579 [details]
Bug 1387243 Part 1: Split nsDocument::EndLoad into a part that runs unconditionally, and a part that runs only when there is a matching BeginLoad call.

https://reviewboard.mozilla.org/r/164652/#review172508

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1eb4231c2361
Part 1: Split nsDocument::EndLoad into a part that runs unconditionally, and a part that runs only when there is a matching BeginLoad call. r=bz

Comment 18

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/1eb4231c2361