217000 - calling document.open from a javascript: src= mixed with frames causes silent failures

Status: RESOLVED FIXED

(Core :: Layout: Images, Video, and HTML Frames, defect)

Description

[Christopher Blizzard (:blizzard)]

This is from jst's email:


While loading the testcase you sent me, we end up calling document.open from a
javascript: URL in a src of a frame element, and the call to document.open is
made before the document's presshell is created (i.e. before we start displaying
it). The current code can't quite cope with that, so I made some changes to make
it deal (see attached patch). Not sure how safe this is yet, but it makes your
testcase load, at least mostly. The progress bar still doesn't stop, and I don't
know why yet. It could be due to a problem with javascript: URL's again, or it
could be something else.

At least the silent script error is fixed by the attached patch, but I don't
know if that's enough. Could you apply the attached patch and try it in the
reall webapp and report back what you get with this patch?

Comment 1

[Christopher Blizzard (:blizzard)]

Attachment: patch

Patch from jst.

Comment 2

[Christopher Blizzard (:blizzard)]

jst, do you have any further thoughts about this patch?

Comment 3

[Christopher Blizzard (:blizzard)]

heloooo

Comment 4

[Johnny Stenback (:jst)]

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

AFAICT, this is the right thing to do. Requesting reviews.

Comment 5

[Peter Van der Beken [:peterv]]

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

>Index: content/html/document/src/nsHTMLDocument.cpp
>===================================================================

>-  nsCOMPtr<nsIDocShell> docshell;
>+  nsCOMPtr<nsIDocShell> docshell = do_QueryReferent(mDocumentContainer);
> 
>   // Stop current loads targeted at the window this document is in.
>-  if (mScriptGlobalObject) {
>-    mScriptGlobalObject->GetDocShell(getter_AddRefs(docshell));
>-
>-    if (docshell) {
>-      nsCOMPtr<nsIWebNavigation> webnav(do_QueryInterface(docshell));
>-      webnav->Stop(nsIWebNavigation::STOP_NETWORK);
>-    }
>+  if (docshell) {
>+    nsCOMPtr<nsIWebNavigation> webnav(do_QueryInterface(docshell));
>+    webnav->Stop(nsIWebNavigation::STOP_NETWORK);
>   }

After talking this over we agreed that we don't want this change.

Comment 6

[Christopher Blizzard (:blizzard)]

So what does that mean, exactly?  That the patch is bad?  Or that you just don't
need part of it?

Comment 7

[Johnny Stenback (:jst)]

Hehe, well, the part of the change we don't want is the part that makes us call
Stop() on the nsIWebNavigation if we do have a docshell (in mDocumentContainer),
but no mScriptGlobal. So the part in comment #5 needs to be rewritten to never
get the docshell from the script global, but only call Stop() on the
nsIWebNavigation if mScriptGlobal is non-null.

Comment 8

[Christopher Aillon (sabbatical, not receiving bugmail)]

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

Thought I had already marked this with r=caillon, but bugzilla must not have
picked it up.

Comment 9

[Johnny Stenback (:jst)]

Attachment: Incorporate the discussed change

Comment 10

[Johnny Stenback (:jst)]

Fix checked in on the trunk. If this is needed on a branch, I'd like to see it
sit on the trunk for a while before landing it on a branch.

Comment 11

[Ian Neal]

Has there been a bug created to deal with why the progress bar doesn't stop (and
throbber from the test case I've been using for bug 166328)?

Comment 12

[Ian Neal]

*** Bug 166328 has been marked as a duplicate of this bug. ***

Comment 13

[Ian Neal]

I suspect the outstanding issue is bug 143398 or related to it.