1263060 - Avoid WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80040111 in dom/base/nsFrameLoader.cpp during startup

Status: RESOLVED FIXED

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

Description

[Jonathan Watt [:jwatt]]

In debug builds I get the following:

[87236] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80040111: file /Users/jonathan/moz/trees/i/dom/base/nsFrameLoader.cpp, line 272
[87236] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80040111: file /Users/jonathan/moz/trees/i/dom/base/nsFrameLoader.cpp, line 272

The first failure occurs under:

  nsFrameLoader::MaybeCreateDocShell
  nsFrameLoader::CheckForRecursiveLoad
  nsFrameLoader::CheckURILoad
  nsFrameLoader::LoadURI

when loading chrome://browser/content/tabbrowser.xml because in MaybeCreateDocShell the |!doc->IsActive()| check evaluates to true here:

https://mxr.mozilla.org/mozilla-central/source/dom/base/nsFrameLoader.cpp?rev=577472ad5c38&mark=1864-1868#1864

That causes us to return NS_ERROR_NOT_AVAILABLE up to LoadURI where the NS_ENSURE_SUCCESS fails and issues the warning.

The second failure occurs in the same way loading chrome://browser/content/socialchat.xml

The doc->IsActive() check came from http://hg.mozilla.org/mozilla-central/rev/cee083078957 (bug 610571) FWIW.

Comment 1

[Andrew Overholt [:overholt]]

Do you think a newish contributor could take this, Olli?

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

Hmm, well, the warning is useful for debugging, so I'd prefer to not remove it. 
And the warning seems to hint about some real bug in, I guess in XUL, where we try to start loading too early. So hard to say...someone would need to debug this a bit that where and why we're trying start loading.

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

And that possible bug in XUL (or XBL) is possibly more like an having extra call to the method, where we should just call it once later when docshell is active, so nothing critical.

Comment 4

[Jonathan Watt [:jwatt]]

Attachment: stack

This happens when we create the frame tree for chrome://browser/content/browser.xul and start attaching bindings XBL bindings to that document. I'm not sure what we could reasonably do to change the ordering of when things load.

Comment 5

[Jonathan Watt [:jwatt]]

Attachment: patch

How about we add the check in MaybeCreateDocShell to LoadFrame too. In the instance of this bug that would mean that we return earlier. In this patch I've also fixed up the check in TryRemoteBrowser to be the same as the check in LoadFrame for consistency.

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8753087 [details] [diff] [review]
patch

The change to nsFrameLoader::TryRemoteBrowser() looks good, but not sure about 
nsFrameLoader::LoadFrame(). That certainly changes behavior since we won't be firing error event anymore. Because of that, r-

Comment 7

[Jonathan Watt [:jwatt]]

Attachment: part 1 - Replace redundant nsIDocument::IsResourceDoc() checks in nsFrameLoader with asserts

Comment 8

[Jonathan Watt [:jwatt]]

Attachment: part 2 - Prevent nsFrameLoader from loading sub-documents is XBL bindings documents

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8753470 [details] [diff] [review]
part 1 - Replace redundant nsIDocument::IsResourceDoc() checks in nsFrameLoader with asserts


> nsFrameLoader::Create(Element* aOwner, bool aNetworkCreated)
> {
>   NS_ENSURE_TRUE(aOwner, nullptr);
>   nsIDocument* doc = aOwner->OwnerDoc();
>+
>+  // XXXjwatt: We really need an explanitory comment here.  The following check
>+  // prevents us from creating nsFrameLoaders for elements that belong to a
>+  // data document or that are not "in a shadow-including document", _unless_
>+  // they belong to a static document.  Why do static documents need
>+  // out-of-composed-tree elements to have nsFrameLoaders, and why do static
>+  // documents that are data documents need elements to have nsFrameLoaders?
>+  // Is it even possible for a document to be both a static and data document?
>+  // If so, how?
This isn't really useful comment. Static documents are static, and it is trivial to see
in nsGenericHTMLFrameElement::CopyInnerTo why frameloader creation is different.
So, please don't add the comment, at least not in its current form.

r+ for the rest.

Comment 10

[Jonathan Watt [:jwatt]]

(In reply to Olli Pettay [:smaug] from comment #9)
> This isn't really useful comment.

My source comment doesn't answer questions, but it poses questions that would be very useful to have answered there to make some sense of code that otherwise seems to make no sense at all. The hope would be that someone would figure out the answers and document them at some point (helping others who are trying to understand this code) or else realize the code is wrong and fix it.

> Static documents are static, and it is
> trivial to see
> in nsGenericHTMLFrameElement::CopyInnerTo why frameloader creation is
> different.

Yes, frameloader creation is different for static documents, but the code in nsGenericHTMLFrameElement::CopyInnerTo doesn't answer either of the questions I posed in that comment. Specifically:

 1) Why do static documents need *out-of-composed-tree* elements to have
    nsFrameLoaders?

 2) Why do static documents that are data documents need elements to have
    nsFrameLoaders?

The only way we create static documents seems to be via the nsIDocument::CreateStaticClone call in nsPrintObject::Init. It seems therefore that the answer to #1 is likely "static documents are only created by cloning another document, so they can't contain out-of-composited-tree elements - this code is broken". And it seems like the answer to #2 is "static documents can't be data documents since data documents cannot be printed - this code is broken".

If these answers are wrong and this code is not broken and needs to be this way, it would be very useful to have a comment explaining why. I've spent a fair bit of time trying to figure out the answers, every reader of this code shouldn't be having to do that.

Anyway, sure, I'll drop the comment before checkin.

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b83624df78b3
https://hg.mozilla.org/integration/mozilla-inbound/rev/bb06914d6e8b

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Jonathan Watt [:jwatt] from comment #10)

> The only way we create static documents seems to be via the
> nsIDocument::CreateStaticClone call in nsPrintObject::Init. It seems
> therefore that the answer to #1 is likely "static documents are only created
> by cloning another document, so they can't contain out-of-composited-tree
> elements - this code is broken". 
Don't know what you mean with "this code is broken"

> And it seems like the answer to #2 is
> "static documents can't be data documents since data documents cannot be
> printed - this code is broken".
static documents are static. Used for printing and print preview, and data since they are supposed to not load anything etc.

Comment 13

[Jonathan Watt [:jwatt]]

(In reply to Olli Pettay [:smaug] from comment #12)
> static documents are [snip] data

Ah-hah! The meaning of the various "document is this"/"document is that" methods on nsIDocument may be obvious to you as someone who has written and worked in this code for years, but IsLoadedAsData, IsLoadedAsInteractiveData, and others are completely undocumented, and IsStaticDocument's documentation doesn't mention this. It may seem like I might have guessed that, but it's what I guessed for IsLoadedAsData and IsLoadedAsInteractiveData where it would seem even _more_ obvious, but as it happens that was a _bad_ guess (see bug 1273544).

Anyway, this answers one of the questions I posed. I've spun off bug 1273850 for the other one.

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

I'd say IsLoadedAsInteractiveData() is rather odd case, but data and static less so.
I wonder if IsLoadedAsInteractiveData should be just renamed to IsLoadedAsXBL() or some such.

Comment 15

[Jonathan Watt [:jwatt]]

I think that would be a less confusing name. I'd like to also document its invariants and effects at the same time, but I'd need to figure those out. Anyway, IsLoadedAsInteractiveData specific discussion should happen in bug 1273544.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b83624df78b3
https://hg.mozilla.org/mozilla-central/rev/bb06914d6e8b