Closed Bug 1387243 Opened 7 years ago Closed 7 years ago

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

Categories

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

Product:
Core
Component:
DOM: Core & HTML
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla57
Tracking Flags:
Tracking Status
firefox57 --- fixed

People

(Reporter: bradwerth, Assigned: bradwerth)

References

Details

Attachments

(1 file)

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.
59 bytes, text/x-review-board-request
bzbarsky
: review+
Details 
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.
Blocks: 1299022
Assignee: nobody → bwerth
Attachment #8893579 - Flags: review?(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 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.
Please see questions in comment 7?
Flags: needinfo?(bwerth)
(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 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.
> 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!
Attachment #8893579 - Flags: review?(bzbarsky)
(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.
Flags: needinfo?(bwerth)
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.
Attachment #8893579 - Flags: review+
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
Attachment #8893579 - Flags: review+
Pushed by bwerth@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/1eb4231c2361
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Depends on: 1412173
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: