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! :)

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.

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

On Mon, 07 May 2001 16:37:33 -0600 valeski@netscape.com (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

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.

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.

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.

The first patch has been checked in!!

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;

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

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.

sr=mscott

r=jst

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

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...

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...

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).

Moving off thte 0.9.2 radar...

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?

Is this still a tracker bug as per the summary?

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

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.

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

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

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.

The final patch has been checked into the trunk.

all done...

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;

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.

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

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

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.

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

r/sr=jst

r=vidur.

*now* i've checked in the patch to fix javascript bookmarklet bustage :-) once again, i'm going to try to close this bug out :-)

v