192322 - Calling nsDocShell::GetDocument creates about:blank document if called when doc doesn't exist

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect)

Description

[nws]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.0.0) Gecko/20020530
Build Identifier: trunk

If a call is propagated down to nsDocShell::GetDocument via the embedding apis
(like nsIWebNavigation::GetDocument()) and no document is loaded yet, nsDocShell
creates an "about:blank" document.  Is this proper behavior for what appears to
be an accessor function?  Rather than change the state of the object, shouldn't
it just return null if no document exists?  

Reproducible: Always

Steps to Reproduce:

Comment 1

[Boris Zbarsky [:bzbarsky]]

jst, this sounds like your sort of thing... ;)

Comment 2

[Adam Lock]

See bug 88229

The issue with GetDocument is that it is called from embedding, internally and
from chrome & content via the DOM.  It is preferable that this method *always*
return a document (lazily created on first call), rather than risk crashes or
ill-defined behaviour when the content or an embedder wants a document, e.g. for
document.write operations and there isn't one.

This was particularly a problem for embedders when dealing with popup windows -
they'd open a new window in response to window.open, but the content would
immediately try to document.write to it and fail. Mozilla the app didn't suffer
this problem because new navigator chrome windows specify about:blank into their
content area so document.write always succeeds.

I think this is probably WONTFIX.

Comment 3

[nws]

Okay - I just read over 88229 - I can see there was a lot of swirl to get to
that solution, so no doubt there is a lot depending on this behavior.  :-)  

It just seems kind of unexpected from an embedding api point of view.  Should a
Get method create the object if it doesn't exist or just report that it doesn't
exist?  One example we ran into is in our nsIURIContentListener implementation.
 I have a few places where I check to see if the originating document is a
subframe.  In one case, I was performing the check before the first document had
been created, expected to just get back a null for GetDocument()if it didn't
exist yet. Instead, about:blank was created when I thought I was doing a check.
 (Additional side effects - in the process of about:blank being created, a
listener that had been set at creation time on the nsIDOMEventTarget interface
was removed - which is how we ran into this...)  

Anyhow - we can certainly work around this - it just doesn't seem consistent
with the way other Get methods work.  The behavior should at least be documented
in the interface definitions.

Comment 4

[Dan M]

The intent, as Adam said, is to always return a document when asked for it. As
if the document had always been there. Because it's a very basic part of the
docshell and much code assume its existence. We create it lazily because
normally it isn't necessary to fudge one; the real document is probably loading
asynchronously and only an accident of timing would cause someone to request the
document before the real one had loaded.

On one hand it's arguably an error to create the document lazily. It changes the
system's behaviour and causes side effects like the one Nancy pointed out. On
another hand it's equally an error to immediately create the temporary
about:blank document in all cases: that has the same effect; it just changes the
timing and who's affected.

I agree we should document the interface. I think there's no one implementation
that will surprise no one.

Comment 5

[Adam Lock]

Attachment: Patch

This patch cleans up the documentation for the document attribute and also
fixes the following referingURI attribute, spelling it correctly at long last.

Comment 6

[Dan M]

all comments and spelling. looks beyootiful to me.

Comment 7

[Adam Lock]

Comment on attachment 114100 [details] [diff] [review]
Patch

Dan & Boris / Johnny can I have an r/sr for this please?

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 114100 [details] [diff] [review]
Patch

Looks good.  Since the attr never returns null, maybe at least document the
exceptions that it could throw if it _does_ fail?

Comment 9

[Dan M]

Comment on attachment 114100 [details] [diff] [review]
Patch

Crikey. You spell good. Check 'er in. Boris: error returns in this method are
pretty much limited to the NS_OK, NS_ERROR_FAILURE universe.

Comment 10

[Adam Lock]

Comment on attachment 114100 [details] [diff] [review]
Patch

Requesting 1.3 checkin approval. Changes are limited to documentation changes
and a spelling corrections in an idl attribute.

Comment 11

[Asa Dotzler [:asa]]

Comment on attachment 114100 [details] [diff] [review]
Patch

a=asa (on behalf of drivers) for checkin to 1.3 final.

Comment 12

[Adam Lock]

Fix is checked in. I altered the comment slightly to say 

"Retrieves the current DOM document for the frame, or lazily creates a blank
document if there is none. This attribute never returns null except for
unexpected error situations."