65777 - Loading/Targetting tracker bug.

Status: VERIFIED FIXED

(Core :: DOM: Navigation, defect)

Description

[Eric Pollmann]

This bug was created to track all of the seemingly related issues with document
loading and targetting.  Please add bugs to the dependency list as needed!  :)

Comment 1

[Johnny Stenback (:jst)]

Nominating for mozilla0.9.1, catfood, please see Dan Rosen's comments in bug
73203 for arguments to get this fundamental flaw fixed in the dochsell code. I
talked to Rick Potts about this a few days ago and he has thoughts about how to
fix this problem.

Comment 2

[rpotts (gone)]

Attachment: Patch which addresses most of the targeting issues...

Comment 3

[rpotts (gone)]

Attaching Jud's comments while reviewing the above patch...
-- rick

Jud wrote:

Got some questions:

regarding docshell mods:
- you've commented out the static security manager CID definition... can you
just delete the line completely?
- your wwatch->OpenWindow() call should check the return value.
- you can rip out the shrimp stuff completely as we no longer ship shrimp
- you commented some DOM_RETVAL_UNDEF handling, can it be removed completely? it
sounded like this was dead code when we were talking on the phone
- should we treat "_new" propegation for context menu "open in new window"
handling as a seperate issue and handle it in another bug?

Jud

Comment 4

[rpotts (gone)]

On Mon, 07 May 2001 16:37:33 -0600
 [e-mail] (Judson Valeski) wrote:
> Got some questions:
>
> regarding docshell mods:
> - you've commented out the static security manager CID
> definition... can
> you just delete the line completely?

Done...

> - your wwatch->OpenWindow() call should check the return
> value.

Oops. You're right! I've added more error checking around
that block of code :-)

> - you can rip out the shrimp stuff completely as we no
> longer ship shrimp

I'm afraid!! I know that as soon as I delete it, someone is
going to start screaming :-) What if I delete it as a
separate bug? That way it will be more obvious that it is
going away, and didn't just get "lost" in all the targeting
changes...

> - you commented some DOM_RETVAL_UNDEF handling, can it be
> removed
> completely? it sounded like this was dead code when we
> were talking on
> the phone

Done... I left the DOM_RETVAL stuff in the URILoader to
"remind" me to fix it :-)

> - should we treat "_new" propegation for context menu
> "open in new
> window" handling as a seperate issue and handle it in
> another bug?
>
I believe that this is bug #77750. It is marked as
depending on bug #65777 and I'll take care of it as soon as
these changes land...

-- rick

Comment 5

[Scott MacGregor]

man that was a big diff =). sr=mscott modulo Judson's comments which it sounds
like you've already addressed in your tree. I'm looking forward to these changes.

Comment 6

[R.K.Aa.]

I wonder if the recent checkins caused bug 80667. Also, i can no longer read the
country's largest newspaper http://www.aftenposten.no cause i'm being forwarded
to one of the links on the front page shortly after it's started to render.

Comment 7

[Asa Dotzler [:asa]]

This morning's builds had a bug ( bug 80746 ) which may have led to 
a Bugzilla user inadvertantly 
changing this bug from the Assignbed/Accepted status to the New status.  If you 
are the owner of this bug please check to see that it is in the correct Status.  
Thanks.

Comment 8

[rpotts (gone)]

The first patch has been checked in!!

Comment 9

[serge (gone)]

chrome://blah.blah.blah does crash:(
the next check will help:
---
RCS file: /cvsroot/mozilla/docshell/base/nsDocShell.cpp,v
retrieving revision 1.296
diff -u -6 -r1.296 nsDocShell.cpp
--- nsDocShell.cpp      2001/05/14 02:12:10     1.296
+++ nsDocShell.cpp      2001/05/17 00:33:58
@@ -3906,12 +3906,13 @@

     rv = NS_OpenURI(getter_AddRefs(channel),
                     aURI,
                     nsnull,
                     loadGroup,
                     NS_STATIC_CAST(nsIInterfaceRequestor *, this));
+    if (NS_FAILED(rv)) return rv;

Comment 10

[Johnny Stenback (:jst)]

I checked in the above patch, is there more to this bug or should it be marked
fixed now?

Comment 11

[rpotts (gone)]

Attachment: Patch to avoid creating intermediate windows for mailto://

Comment 12

[rpotts (gone)]

I've just attached another patch which addresses the problem of creating
intermediate windows for targeted mailto:// URLs...  This patch causes the new
window to be destroyed if the channel returns NS_ERROR_NO_CONTENT - indicating
that no content is available for the URI...

Any protocol which is used as a "command" rather than a "data-source" should
return NS_ERROR_NO_CONTENT.

Comment 13

[Scott MacGregor]

sr=mscott

Comment 14

[Johnny Stenback (:jst)]

r=jst

Comment 15

[rpotts (gone)]

I've checked in the patch for mailto:// URLs that are explicitly target at a new
window...

Comment 16

[rpotts (gone)]

The only window targeting work that is left is to rework the javascript protocol
handler so that it returns NS_ERROR_NO_CONTENT when AsyncOpen() is called...

This will allow me to clean up the URILoader a bit and remove the
NS_ERROR_DOM_RETVAL_UNDEFINED error code...

Comment 17

[rpotts (gone)]

I'm moving this bug out of the current milestone (0.9.1) because I don't think
that the javascript protocol handler work is critical...

Comment 18

[rpotts (gone)]

Attachment: Patch to make javascript: URLs evaluate synchronously when AsyncOpen is called...

Comment 19

[rpotts (gone)]

I've just attached a (rather large) patch which reworks the javascript: protocol
handler a bit...

1. Script evaluation is now performed synchronously when either
nsIChannel::AsyncOpen(...) or nsIChannel::Open(...) is called.

2. The nsEvaluateStringProxy object has been removed.  It is not needed because
script evaluation is done inside of the channel's open calls (on the UI thread).

3. A custom javascript channel has been added (nsJSChannel) which wraps a necko
nsIOStreamChannel.  Providing a dedicated channel implementation allows script
execution to occur synchronously in AsyncOpen (where it belongs).

Comment 20

[rpotts (gone)]

Moving off thte 0.9.2 radar...

Comment 21

[Johnny Stenback (:jst)]

Rick, in nsJSThunk::Open() you do this:

+
+        return mLength ? NS_OK : NS_ERROR_DOM_RETVAL_UNDEFINED;

What if we execute javascript:"", that'll result in an empty string, which
should be "parsed" and presented to the user :-)

Could we use mResult to indicate if there was a result or not?

Comment 22

[lchiang]

Is this still a tracker bug as per the summary?   

Comment 23

[rpotts (gone)]

right now, it is both a "tracker" bug (for bug #56067) and a "real" bug for the 
javascript: protocol handler patch :-(

after I landed the first set of patches, i had hoped to quickly close this bug 
out... now, i hope to close it after i land the patch to javascript:

-- rick

Comment 24

[lchiang]

Thanks for the response, rpotts.
Per PDT team triage, removing PDT+ for this bug as it is not a stopper bug.  If
you feel this fix should be in a "limbo" branch build, pls mark nsBranch in the
keyword field.  Thanks.

Comment 25

[rpotts (gone)]

thanks johnny,

I didn't realize that a javascript URL could return empty content, but still
have it rendered :-)

In that case, I think that nsJSThunk::Open() should *always* return NS_OK.

If there *really* is no content, then it will be caught in
nsJSChannel::AsyncOpen() since the return value from EvaluateScript() will be
NS_ERROR_DOM_RETVAL_UNDEFINED.  This will prevent Open() from being called in
the first place...

I'm attaching a new patch which does that...  It changes nsJSThunk::Open() to
always return NS_OK;

-- rick

Comment 26

[rpotts (gone)]

Attachment: new patch which addresses jst's issues...

Comment 27

[Johnny Stenback (:jst)]

There's an aweful lot of inlined code in nsJSThunk which should be moved outside
the class declaration, for clarity if nothing else, but either way, sr=jst

Comment 28

[vidur (gone)]

The nsIChannel::SetContentType call in the thunk's EvaluateScript method is
probably redundant since the content type is set again when the thunk's Open
method is invoked.

+            rv = mChannel->SetContentType("text/html");

Other than that, r/sr=vidur.

Comment 29

[rpotts (gone)]

Attachment: final patch - removed SetContentType() and moved inlined methods out-of-line

Comment 30

[rpotts (gone)]

The final patch has been checked into the trunk.

Comment 31

[rpotts (gone)]

all done...

Comment 32

[jag (Peter Annema)]

This broke adding the result of javascript bookmarklets to session history on
linux, and from what I can tell broke them completely on Windows. Reopening this
bug.

I have a couple of js bookmarklets (I'll paste them below) which I use to
quickly look up a file in lxr, or a bug in bugzilla. This used to work fine, and
add the generated URL to the session history (so one can move back/forward to
them) until a few days ago. Since then, on linux it loads the js bookmarklet (in
my case popping up a dialog), and on windows it doesn't load the resulting url
half of the time.

Binary searching in nightly builds, then consulting bonsai and trying backing
out a few patches, it turns out this patch is what causes it.

Find file with LXR:
javascript:var file=prompt("LXR File","");if (file)
location.href="http://lxr.mozilla.org/seamonkey/find?string="+file;

Find text with LXR:
javascript:var text=prompt("LXR Text","");if (text)
location.href="http://lxr.mozilla.org/seamonkey/search?string="+escape(text);

Go to bug:
javascript:var num=prompt("Bug#","");if (num)
location.href="http://bugzilla.mozilla.org/show_bug.cgi?id="+num;

Comment 33

[jag (Peter Annema)]

Btw, what I think should happen, but is probably another bug, is that both the
javascript bookmarklet and the resulting url are added to session history, so
that going back from the "generated" location triggers the javascript to run
again.

Comment 34

[rpotts (gone)]

Attachment: patch to fix regression of javascript bookmarklets

Comment 35

[rpotts (gone)]

moving milestone -> 1.0  Since these patches live on the trunk only.

Comment 36

[rpotts (gone)]

I've checked in a patch which should fix the regressions i introduced to
javascript bookmarkets...

The problem was one of ordering.  i didn't take into account that the script
itself would (possible) load a document *and* produce output :-(  When this
occurred the javascript output cancelled the new document load (incorrectly)
because when the document was loaded, the javascript channel was not part of the
loadgroup (allowing it to be cancelled)...

-- rick

Comment 37

[jag (Peter Annema)]

ITYM "attached", not "checked in".

Any chance you can check this in on the trunk soon? I think 0.9.3 is still a
valid milestone for the trunk (afaik), so I don't see why you'd have to postpone
this till 1.0.

Comment 38

[rpotts (gone)]

i'll try...  as soon as I get some reviews, i'll check it into the trunk.

Comment 39

[Johnny Stenback (:jst)]

r/sr=jst

Comment 40

[vidur (gone)]

r=vidur.

Comment 41

[rpotts (gone)]

*now* i've checked in the patch to fix javascript bookmarklet bustage :-)

once again, i'm going to try to close this bug out :-)

Comment 42

[Daniel Wang]

v