205989 - bookmarklets that end with location=foo don't work correctly

Status: VERIFIED FIXED

(SeaMonkey :: General, defect)

Description

[Asa Dotzler [:asa]]

Jesse Ruderman's collect buglinks bookmarklet no longer functions properly for
me in Mozilla Firebird (formerly Phoenix) and I've just read a report at
mozillaZine that others are having problems with bookmarklets (in SeaMonkey, I
think). 

This bookmarklet should analyze the page for bugzilla bug URLs and then load a
bugzilla buglist but the list isn't loaded, the URL is just displayed in the
content area.

javascript:y = ''; for (i = 0; i < document.links.length; ++i) { L =
document.links[i]; x = L.href; a = L.innerHTML; if (x.substring(0,44) ==
'http://bugzilla.mozilla.org/show_bug.cgi?id=' && x != document.location.href &&
a != 'First' && a != 'Next' && a != 'Prev' && a != 'Last') y += ',' +
parseInt(x.substring(44,61)) } if(y.length) document.location.href=
'http://bugzilla.mozilla.org/buglist.cgi?bug_id=' + y; else alert('no buglinks
on this page');

This isn't broken for SeaMonkey but is for Firebird. The report at mozillaZine
seems to be for SeaMonkey.

The mozillazine thread discussing a broken bookmarklet is at
http://www.mozillazine.org/forums/viewtopic.php?t=10840

Comment 1

[Asa Dotzler [:asa]]

maybe related to the changes for bug 130265 ?

Comment 2

[Johnny Stenback (:jst)]

Hmm, I wonder if those bookmarklets even work in 4x or IE (if it even supports
bookmarklets). Fixing the bookmarklet is easy, just make sure that it doesn't
result in any data, i.e. add void(0); at the end of the javascript: URL.

I'm arguing that this isn't broken, it's supposed to work this way, the fact
that it didn't earlier, was IMO a bug.

Comment 3

[Johnny Stenback (:jst)]

This is fixable, though, so let me know if you want a fix.

Comment 4

[Jesse Ruderman]

These two bookmarklets:
javascript:location='http://slashdot.org/';
javascript:location='http://slashdot.org/'; 4
take you to http://slashdot.org/ in IE, Opera 7, and Netscape 4.  It seems like
setting location= overrides the "last value calculated" even though it does not
halt the javascript.

This type of bookmarklet is common.  Most "search bookmarklets" (e.g. select a
word and click the bookmarklet to search Google for it) are of this form.

I think http://www.mozillazine.org/forums/viewtopic.php?t=10840 is a separate
problem.  It was first posted two days ago and involves a bookmarklet that ends
with a void.

Comment 5

[Johnny Stenback (:jst)]

Taking, I've got a fix.

Comment 6

[Johnny Stenback (:jst)]

Attachment: Proposed fix. Don't parse javascript: URL data if location was set.

This should take care of this, but given the regression history for this code,
who knows. I've tested all kinds of bookmarklets I can think of, and standalong
javascript: links of various sorts, so I *think* this is good.

Comment 7

[Darin Fisher]

Comment on attachment 123558 [details] [diff] [review]
Proposed fix. Don't parse javascript: URL data if location was set.

jst: this patch makes good sense to me.  i like the fact that you are
separating mStreamChannel more from the nsJSChannel.

i worry about the change in GetStatus... be sure to verify that bug 193887 is
still fixed (that bug is the reason why it was important to call Cancel on
mStreamChannel from AsyncOpen).  however, given the changes in GetStatus... i
wonder if you can do away with the bogus call to Cancel on mStreamChannel?  it
never made much sense to me anyways.

Comment 8

[Adam Lock]

Comment on attachment 123558 [details] [diff] [review]
Proposed fix. Don't parse javascript: URL data if location was set.

Looks okay, but is it true that javascript: is never the actual document URI,
even when loaded from the address bar?

Comment 9

[Johnny Stenback (:jst)]

Daring, I checked if this regresses bug 193887, and it looks like it doesn't, so
that part should be fine. I but a breakpoint in ::GetStatus() and I never saw
that being called, so I'm assuming that the fact that we're now pulling the
nsJSChannel out of the loadgroup before opening the real stream means that
::GetStatus() won't be called on the nsJSChannel, at least not in any normal
cases that I was able to manufacture.

Adam, the javascript: URI can be the document URI, but the nsJSChannel itself is
never the 'document channel', that's always the inner stream channel, or no
channel at all if there's no data to parse from the javascript: URI.

Comment 10

[Johnny Stenback (:jst)]

Oh, and Darin, I checked to see what we actually do in the call to
mStreamChannel->Cancel() in ::AsyncOpen(), and we never do anything in the cases
I tested, but I'd feel better leaving that in for now (given how close to a
release we are).

Comment 11

[Johnny Stenback (:jst)]

Comment on attachment 123558 [details] [diff] [review]
Proposed fix. Don't parse javascript: URL data if location was set.

Requesting approval for 1.4 final. This is a fix for a serious regression
caused by the fix for bug 130265.

Comment 12

[Johnny Stenback (:jst)]

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

Comment 13

[Jesse Ruderman]

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

Comment 14

[Jesse Ruderman]

Since this bug has dups from people outside the security group, I think it
should not be marked as security-sensitive.

Comment 15

[Johnny Stenback (:jst)]

I'll let mstoltz make that decision.

Comment 16

[Asa Dotzler [:asa]]

Why was this bug marked as security confidential? I didn't file it as confidential. 

Comment 17

[Brendan Eich [:brendan]]

Nominating bug for 1.4 -- another driver should a= the patch once darin sr's.

/be

Comment 18

[Darin Fisher]

Comment on attachment 123558 [details] [diff] [review]
Proposed fix. Don't parse javascript: URL data if location was set.

>+nsJSChannel::InternalOpen(PRBool aIsAsync, nsIStreamListener *aListener,
>+                          nsISupports *aContext, nsIInputStream **aResult)
...
>+    // Add the javascript channel to its loadgroup so that we know if
>+    // network loads were canceled or not...
>+    mStreamChannel->GetLoadGroup(getter_AddRefs(loadGroup));
>+    if (loadGroup) {
>+        (void) loadGroup->AddRequest(this, aContext);
>+    }

i'm drawing a bit of a blank here, but wasn't there a case in bug 130265
in which we needed to not add the nsJSChannel to the load group?  maybe
you've resolved that issue in a different way?	i understand why you need
to add nsJSChannel to the loadgroup now, so probably i'm just forgetting
the cases from the older bug.

sr=darin

Comment 19

[Johnny Stenback (:jst)]

The case where I did *not* want to add the nsJSChannel to the loadgroup in bug
130265 was that it could have the LOAD_DOCUMENT_URI load flag set and having
that in the loadgroup made the assert you metntioned in the document loader
fire. Here I never set that load flag on the nsJSChannel (but I do pass it to
the inner stream channel, i.e. the opposite of what this code used to do) so
it's 'safe' to add the nsJSChannel to the loadgroup now. And note that I pull
the nsJSChannel out of the loadgroup here right after the JS is evaluated,
*before* the inner stream channel is opened, just to make it not interfere more
than it needs to. The sole reason for adding the nsJSChannel to the loadgroup is
to catch the fact that evauating the JS in the javascript: URL can stop network
traffic, and we need to know that, so the nsJSChannel now remembers that and
avoids opening the inner stream channel if network traffic was stopped from the JS.

Comment 20

[Jesse Ruderman]

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

Comment 21

[Jesse Ruderman]

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

Comment 22

[Asa Dotzler [:asa]]

Comment on attachment 123558 [details] [diff] [review]
Proposed fix. Don't parse javascript: URL data if location was set.

a=asa (on behalf of drivers) for checkin to 1.4.

Is there some sensitive information in this bug that isn't also available in
any of the independently reported, and still public, dupes?

Comment 23

[Mitchell Stoltz (not reading bugmail)]

Removing security-sensitive as I don't see any security threat here. If I've
missed something important, please let me know.

Comment 24

[Johnny Stenback (:jst)]

Fix checked in. Marking FIXED.

Comment 25

[Erik]

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

Comment 26

[Asa Dotzler [:asa]]

My collect buglinks bookmarklet is working in today's Firebird build 20030522.
Tested on RH-9 linux.

Comment 27

[Simon Paquet [:sipaq]]

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

Comment 28

[Erik]

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

Comment 29

[Jesse Ruderman]

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

Comment 30

[Erik]

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

Comment 31

[Erik]

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