357437 - [FIX]Middle clicking bookmark with javascript URL fails

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect, P1)

Description

[Ria Klaassen (not reading all bugmail)]

Steps to reproduce:

1. Make bookmark with something like javascript:location.href='http://forums.mozillazine.org/viewforum.php?f=19'
2. Middle click it in order to open it in a new tab

Result: fails (opens empty tab instead)
Expected result: should open bookmark in new tab like usually

Errors like:

Error: location is not defined
Source File: location.href='http://forums.mozillazine.org/viewforum.php?f=19'
Line: 1

or 

Error: window is not defined
Source File: q=(window.getSelection)?window.getSelection():window.selection.createRange().text;if(!q)q=prompt('Search:','');if(q)location.href='http://www.google.nl/search?q='+escape(q);
Line: 1


Regression between 1.9a1_2006101614 and 1.9a1_2006101623:
http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&date=explicit&mindate=2006-10-16+13%3A00&maxdate=2006-10-17+00%3A00

Comment 1

[Ria Klaassen (not reading all bugmail)]

To make it more complicated: it does not *always* fail, but mostly.

Comment 2

[Boris Zbarsky [:bzbarsky]]

So the problem is that we start the about:blank load in the new tab from the frame loader, then start the javascript: load (which picks up the _current_ about:blank principal, the one from CreateAboutBlankContentViewer()).  The javascript: load doesn't stop the about:blank load (due to the patch for bug 130265, which I still think is wrong... because it makes us incompatible with other browsers).  Then the about:blank load completes, then the javascript: load completes.  Now the principals don't match, so we evaluate the javascript: load in a sandbox and you get the described problem.

Oh, the reason there's an in-progress about:blank load is that web content expects <iframe onload="foo()"> (without an src attr) to fire the onload handler.

Solutions I see:

1)  Make javascript: stop other loads like it does in IE (back out bug 130265).
2)  Stop doing fake about:blank loads for chrome (or XUL, or "chrome XUL")
    iframes.
3)  Change whatever code runs onclick here to stop() the load.  Probably the
    tabbrowser's addTab() should do that, in fact...

Thoughts?

Comment 3

[Johnny Stenback (:jst)]

1) sounds like the best approach here IMO. I don't remember the details about that bug, even after reading through a good bit of it, but compatibility with IE and other browser outweighs quite a lot, so I'd vote for this approach.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Well...  As far as compat goes I'm not sure it gives compat with Opera, and I'm not sure about IE7.  It does give compat with IE6, but I recall hearing rumors that the IE7 behavior is different.

What that bug was about, fwiw, is that clicking a javascript: link (or a bookmarklet) stopped the current document load even if the link just ran a function that did nothing else...

Comment 5

[Jesse Ruderman]

I like the fact that bug 130265 is fixed -- it lets me click things like bookmarklets and "javascript:window.open()" links in web pages without stopping the page load.  So I'd prefer (2) or (3), I guess.

Comment 6

[[e-mail]]

(In reply to comment #2)
> 3)  Change whatever code runs onclick here to stop() the load.  Probably the
>     tabbrowser's addTab() should do that, in fact...
Doesn't seem to help. I also tried adding stop() to nsBrowserAccess too.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: Fix

This does option #3.  As Jesse pointed out, this is all a non-issue for trusted content, since about:blank inherits the parent principal there.  So this is probably the way to go, unless people feel #2 is better.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 244018 [details] [diff] [review]
Fix

Gavin, wanna review the toolkit change?

Neil, same for xpfe?

Comment 9

[Boris Zbarsky [:bzbarsky]]

Ctho, whatever browser settings let you have that mid-air... please fix them.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Fixed.

Comment 11

[Jan Moesen]

Could you also check this in on the 1.8 branch? This problem is still present in the latest Firefox 2.0.0.2 RC.

http://forums.mozillazine.org/viewtopic.php?t=502130

Comment 12

[Boris Zbarsky [:bzbarsky]]

That's up to the module owners.  I don't feel confident enough in this patch to land it on stable branches.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Actually, the original problem reported here was a trunk-only regression, so whatever you're seeing in that topic thread is a different bug.

If it's a regression on the branch from 2.0.0.1 (which is what th thread says), please file the new bug and cc me.  If you can figure out the regression range on branch, that would go a long way toward resolving the issue.