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

VERIFIED FIXED in mozilla1.4final

Status

SeaMonkey
General
VERIFIED FIXED
15 years ago
8 years ago

People

(Reporter: asa, Assigned: jst)

Tracking

({regression})

Trunk
mozilla1.4final
regression
Bug Flags:
blocking1.4 +

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment)

(Reporter)

Description

15 years ago
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
(Reporter)

Comment 1

15 years ago
maybe related to the changes for bug 130265 ?
(Assignee)

Comment 2

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

Comment 3

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

Comment 4

15 years ago
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.
Summary: javascript bookmarklets stopped working → bookmarklets that end with location=foo don't work correctly
(Assignee)

Comment 5

15 years ago
Taking, I've got a fix.
Assignee: general → jst
Hardware: PC → All
Target Milestone: --- → mozilla1.4final
(Assignee)

Comment 6

15 years ago
Created attachment 123558 [details] [diff] [review]
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.
(Assignee)

Updated

15 years ago
Attachment #123558 - Flags: superreview?(darin)
Attachment #123558 - Flags: review?(adamlock)

Comment 7

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

15 years ago
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?
Attachment #123558 - Flags: review?(adamlock) → review+
(Assignee)

Comment 9

15 years ago
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.
Group: security
Status: NEW → ASSIGNED
(Assignee)

Comment 10

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

Comment 11

15 years ago
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.
Attachment #123558 - Flags: approval1.4?
(Assignee)

Comment 12

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

Comment 13

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

Comment 14

15 years ago
Since this bug has dups from people outside the security group, I think it
should not be marked as security-sensitive.
(Assignee)

Comment 15

15 years ago
I'll let mstoltz make that decision.
(Reporter)

Comment 16

15 years ago
Why was this bug marked as security confidential? I didn't file it as confidential. 
Nominating bug for 1.4 -- another driver should a= the patch once darin sr's.

/be
Flags: blocking1.4+

Comment 18

15 years ago
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
Attachment #123558 - Flags: superreview?(darin) → superreview+
(Assignee)

Comment 19

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

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

Comment 21

15 years ago
*** Bug 206072 has been marked as a duplicate of this bug. ***
(Reporter)

Comment 22

15 years ago
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?
Attachment #123558 - Flags: approval1.4? → approval1.4+
Removing security-sensitive as I don't see any security threat here. If I've
missed something important, please let me know.
Group: security
(Assignee)

Comment 24

15 years ago
Fix checked in. Marking FIXED.
Status: ASSIGNED → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → FIXED

Comment 25

14 years ago
*** Bug 206573 has been marked as a duplicate of this bug. ***
(Reporter)

Comment 26

14 years ago
My collect buglinks bookmarklet is working in today's Firebird build 20030522.
Tested on RH-9 linux.
*** Bug 206989 has been marked as a duplicate of this bug. ***

Comment 28

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

Comment 29

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

Comment 30

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

Comment 31

14 years ago
*** Bug 218118 has been marked as a duplicate of this bug. ***
Product: Browser → Seamonkey

Updated

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