calling from a javascript: src= mixed with frames causes silent failures

RESOLVED FIXED in mozilla1.6alpha



Layout: HTML Frames
15 years ago
15 years ago


(Reporter: blizzard, Assigned: jst)



Firefox Tracking Flags

(Not tracked)


(Whiteboard: [HAVE FIX])


(2 attachments)



15 years ago
This is from jst's email:

While loading the testcase you sent me, we end up calling from a
javascript: URL in a src of a frame element, and the call to 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

15 years ago
Created attachment 130230 [details] [diff] [review]

Patch from jst.

Comment 2

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

Comment 3

15 years ago

Comment 4

15 years ago
Comment on attachment 130230 [details] [diff] [review]

AFAICT, this is the right thing to do. Requesting reviews.
Attachment #130230 - Flags: superreview?(peterv)
Attachment #130230 - Flags: review?(caillon)


15 years ago
OS: Linux → All
Hardware: PC → All
Whiteboard: [HAVE FIX]
Target Milestone: --- → mozilla1.6alpha
Comment on attachment 130230 [details] [diff] [review]

>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.
Attachment #130230 - Flags: superreview?(peterv) → superreview+

Comment 6

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

Comment 7

15 years ago
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 on attachment 130230 [details] [diff] [review]

Thought I had already marked this with r=caillon, but bugzilla must not have
picked it up.
Attachment #130230 - Flags: review?(caillon) → review+

Comment 9

15 years ago
Created attachment 131764 [details] [diff] [review]
Incorporate the discussed change

Comment 10

15 years ago
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.
Last Resolved: 15 years ago
Resolution: --- → FIXED

Comment 11

15 years ago
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

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

Comment 13

15 years ago
I suspect the outstanding issue is bug 143398 or related to it.
You need to log in before you can comment on or make changes to this bug.