1273850 - Figure out and document why nsFrameLoader::Create returns null if aOwner->IsInComposedDoc() returns false _except_ for static documents

Status: RESOLVED FIXED

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

Description

[Jonathan Watt [:jwatt]]

Spinning this off from bug 1263060 comment 10.

nsFrameLoader::Create contains:

  nsIDocument* doc = aOwner->OwnerDoc();
  NS_ENSURE_TRUE(!doc->IsResourceDoc() &&
                 ((!doc->IsLoadedAsData() && aOwner->GetComposedDoc()) ||
                   doc->IsStaticDocument()),
                 nullptr);

I would like to understand why nsFrameLoader::Create makes an exception for static documents when it otherwise returns null if !aOwner->IsInComposedDoc(), and I think we should really document this in a comment.

The only way we create nodes belonging to a static document seems to be via nsPrintObject::Init when it calls nsIDocument::CreateStaticClone. If nodes that belong to a static document are only created by cloning a _document_, surely all nodes belonging to a static document must be in-composed-document. After all, surely we only clone in-composed-document nodes when we clone a document. If that is the case then the exception in nsFrameLoader::Create to allow for out-of-composed-document elements in the case of a static document seems redundant and confusing.

Even if we could create static clones of out-of-composed-document trees I still don't see how the exception in nsFrameLoader::Create would make sense. The element that was cloned would have been prevented from getting a nsFrameLoader by nsFrameLoader::Create (since it doesn't belong to a static document, and wouldn't trigger the exception), so surely the corresponding elements in the static clone should also be prevented from getting an nsFrameLoader too. Otherwise the static clone could potentially have sub-documents that aren't in the original document, and then it's not much of a clone.

Comment 1

[Jonathan Watt [:jwatt]]

Attachment: some bug archeology using https://github.com/mozilla/gecko-dev

It seems like this exception first started showing up in Olli's v14 patch in bug 487667, but it's not clear why.

Comment 2

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

Because we create the frameloader during element cloning, and at that point we aren't in document yet.
We could possibly move frameloader creation to BindToTree time, but we'd still need checks for static documents, since those aren't supposed to load anything, yet have frameloaders so that we can keep the relevant docshells alive.

Comment 3

[Jonathan Watt [:jwatt]]

(In reply to Olli Pettay [:smaug] from comment #2)
> Because we create the frameloader during element cloning, and at that point
> we aren't in document yet.

This is useful info. And presumably there are other mechanisms that lazy create nsFrameLoader for out-of-tree elements that are not in a static document, which is why we don't create them for other out-of-tree cases, but these mechanisms don't apply to static documents, so we need to special case them.

> We could possibly move frameloader creation to BindToTree time

To me (not knowing the details) that would seem like a desirable change, and like it would make the code more consistent between different scenarios.

> have frameloaders so that we can keep the relevant docshells alive.

It's also interesting to know that this is the reason that nsFrameLoader::Create is even creating nsFrameLoaders for static documents (that wasn't obvious to me). Also worth adding to a comment in that method.

I guess before adding any documenting comments it would be good to know if frameloader creation is likely to move to BindToTree time. If it is, maybe we should hold off on adding those comments.

Comment 4

[Jonathan Watt [:jwatt]]

Attachment: patch

Actually here's some documentation. We can always change it in the nsFrameLoader creation behavior changes.

I've also fixed the indentation of the NS_ENSURE_TRUE, and switched the GetComposedDoc() call to an IsInComposedDoc() call now that the latter exists.

Comment 5

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

(In reply to Jonathan Watt [:jwatt] from comment #3)
> (In reply to Olli Pettay [:smaug] from comment #2)
> > Because we create the frameloader during element cloning, and at that point
> > we aren't in document yet.
> 
> This is useful info. And presumably there are other mechanisms that lazy
> create nsFrameLoader for out-of-tree elements 
Not sure what this means, but usually nsFrameLoader is created during BindToTree.
Static documents are just a special case, since there we don't actually load anything anywhere, but 
just clone the document tree for print preview / printing.


> To me (not knowing the details) that would seem like a desirable change, and
> like it would make the code more consistent between different scenarios.
Right. We'd need to prevent loading anything in nsFrameLoader though, and just
wait for cloning to happen to the inner document.


> I guess before adding any documenting comments it would be good to know if
> frameloader creation is likely to move to BindToTree time. If it is, maybe
> we should hold off on adding those comments.
I don't have plans atm to change static document setup since it has worked pretty well. But I certainly don't object moving nsFrameLoader creation to BindToTree also in that case if it doesn't complicate the code too much.

Comment 6

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

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

thanks

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6523dd4df90d

Comment 8

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

https://hg.mozilla.org/mozilla-central/rev/6523dd4df90d