If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Loading/Targetting tracker bug.

VERIFIED FIXED in mozilla1.0

Status

()

Core
Document Navigation
VERIFIED FIXED
17 years ago
14 years ago

People

(Reporter: Eric Pollmann, Assigned: rpotts (gone))

Tracking

Trunk
mozilla1.0
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(6 attachments)

(Reporter)

Description

17 years ago
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!  :)
(Reporter)

Updated

17 years ago
Depends on: 61187, 61385

Updated

17 years ago
Depends on: 48759

Updated

17 years ago
Blocks: 73203
(Reporter)

Updated

17 years ago
Depends on: 49682
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.
Keywords: correctness, mozilla0.9.1, nsCatFood
(Assignee)

Updated

17 years ago
Blocks: 64775
(Assignee)

Updated

17 years ago
Blocks: 52772
Target Milestone: --- → mozilla0.9.1
(Assignee)

Updated

17 years ago
No longer blocks: 52772
(Assignee)

Updated

17 years ago
Blocks: 56067
Status: NEW → ASSIGNED
(Assignee)

Updated

17 years ago
Blocks: 61176
(Assignee)

Updated

17 years ago
No longer blocks: 56067, 61176, 64775, 73203
Component: Networking → Embedding: Docshell
(Assignee)

Updated

17 years ago
Depends on: 56067, 61176, 64775, 73203
(Assignee)

Updated

17 years ago
Depends on: 68955
(Assignee)

Updated

17 years ago
Depends on: 76408
(Assignee)

Updated

17 years ago
No longer depends on: 76408
(Assignee)

Updated

17 years ago
Depends on: 76408
(Assignee)

Comment 2

17 years ago
Created attachment 33437 [details] [diff] [review]
Patch which addresses most of the targeting issues...

Updated

17 years ago
Depends on: 78545
(Assignee)

Comment 3

17 years ago
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
(Assignee)

Updated

17 years ago
Blocks: 77750
(Assignee)

Comment 4

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

Comment 5

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

17 years ago
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.
Status: ASSIGNED → NEW

Comment 7

17 years ago
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.
(Assignee)

Comment 8

17 years ago
The first patch has been checked in!!
(Assignee)

Updated

17 years ago
Depends on: 52772

Comment 9

17 years ago
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;

Updated

17 years ago
Blocks: 75664
I checked in the above patch, is there more to this bug or should it be marked
fixed now?
(Assignee)

Comment 11

17 years ago
Created attachment 35199 [details] [diff] [review]
Patch to avoid creating intermediate windows for mailto://
(Assignee)

Comment 12

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

17 years ago
sr=mscott
r=jst
(Assignee)

Comment 15

17 years ago
I've checked in the patch for mailto:// URLs that are explicitly target at a new
window...
(Assignee)

Comment 16

17 years ago
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...
(Assignee)

Comment 17

17 years ago
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...
Keywords: mozilla0.9.1 → mozilla0.9.2
Target Milestone: mozilla0.9.1 → mozilla0.9.2
(Assignee)

Comment 18

17 years ago
Created attachment 38646 [details] [diff] [review]
Patch to make javascript: URLs evaluate synchronously when AsyncOpen is called...
(Assignee)

Comment 19

17 years ago
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).
Whiteboard: PDT+
(Assignee)

Comment 20

17 years ago
Moving off thte 0.9.2 radar...
Keywords: mozilla0.9.2 → mozilla0.9.3
Target Milestone: mozilla0.9.2 → mozilla0.9.3
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

17 years ago
Is this still a tracker bug as per the summary?   
(Assignee)

Comment 23

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

17 years ago
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.
Whiteboard: PDT+
(Assignee)

Comment 25

17 years ago
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
(Assignee)

Comment 26

17 years ago
Created attachment 40495 [details] [diff] [review]
new patch which addresses jst's issues...
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

17 years ago
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.
(Assignee)

Comment 29

17 years ago
Created attachment 40559 [details] [diff] [review]
final patch - removed SetContentType() and moved inlined methods out-of-line
(Assignee)

Comment 30

17 years ago
The final patch has been checked into the trunk.
(Assignee)

Updated

17 years ago
No longer depends on: 56067
(Assignee)

Comment 31

17 years ago
all done...
Status: NEW → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → FIXED

Comment 32

17 years ago
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;
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Comment 33

17 years ago
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.
(Assignee)

Comment 34

16 years ago
Created attachment 41349 [details] [diff] [review]
patch to fix regression of javascript bookmarklets
(Assignee)

Comment 35

16 years ago
moving milestone -> 1.0  Since these patches live on the trunk only.
Keywords: mozilla0.9.3
Target Milestone: mozilla0.9.3 → mozilla1.0
(Assignee)

Comment 36

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

16 years ago
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.
(Assignee)

Comment 38

16 years ago
i'll try...  as soon as I get some reviews, i'll check it into the trunk.
r/sr=jst

Comment 40

16 years ago
r=vidur.
(Assignee)

Comment 41

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

once again, i'm going to try to close this bug out :-)
Status: REOPENED → RESOLVED
Last Resolved: 17 years ago16 years ago
Resolution: --- → FIXED

Comment 42

14 years ago
v
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.