342504 - [FIX]Tab is blanked when clicking an anchor link identical to the current url

Status: RESOLVED FIXED

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

Description

[pile0nades]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1a3) Gecko/20060621 BonEcho/2.0a3
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1a3) Gecko/20060621 BonEcho/2.0a3

Go to the url and scroll down to zeniko's next post. Click the "I feel so ignored" link, which is the same url and anchor as the current page. The tab is blanked, and about:blank replaces the current page in the back/forward history, meaning the page can't be retreived by going back, then forward.

Reproducible: Always

Steps to Reproduce:
1. Go to the url
2. scroll to zeniko's post a few posts down
3. click the link

Actual Results:  
Tab contents are deleted.

Expected Results:  
Scroll back up to the anchor.

Comment 1

[pile0nades]

Forgot to mention, set set browser.link.open_newwindow to 1 first.

Comment 2

[Simon Bünzli]

Attachment: minimal testcase

Comment 3

[Simon Bünzli]

This has regressed between the 2006021305 and the 2006021404 branch nightlies. My first suspect would be bug 323810.

OTOH, with four months since the regression, this is probably a DUPE.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Yeah, this is a regression from bug 323810.  For some reason, we're doing an anchor scroll even though we've already started loading about:blank.  

I'll look into this when I get back in July, I guess...  Mike, how does that compare with the 1.8.1 schedule?  Should this be blocking 1.8.1?

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: Actually, this seems to do the trick

That said, I think I'd prefer to fix nsIBrowserDOMWindow per the comments; too bad I can't do that on branch.  :(

Also, this patch could use some heavy testing; I've verified that it fixes this bug, but that's about it.

Comment 6

[Johnny Stenback (:jst)]

What if the current webpage is still loading when we hit this? Won't this stop the load and leave the user with a partial document?

Comment 7

[Boris Zbarsky [:bzbarsky]]

Yes, it will.  Of course if we don't do this it'll leave the user with about:blank....

The only way to really deal with that is to not start a load in an already-existing window in the window provider; I suppose I could just go ahead and extend nsIBrowserDOMWindow accordingly with an nsIBrowserDOMWindow_MOZILLA_1_8_BRANCH (and just change it on trunk).

Comment 8

[Benjamin Smedberg]

On IRC we talked about implementing nsIWindowProvider(2)? on the same objects that currently implement nsIBrowserDOMWindow. I tend to like the elegance of this solution, though it may be more complicated on the branch(es) than a simpler addition to nsIBrowserDOMWindow or even a load-then-stop combination. 

Comment 9

[Johnny Stenback (:jst)]

Comment on attachment 226790 [details] [diff] [review]
Actually, this seems to do the trick

sr=jst on this one regardless of whether we end up going this route or not. And sorry for the long wait.

Comment 10

[Boris Zbarsky [:bzbarsky]]

I really think we should fix this, on both trunk and branches.  Is there anyone familiar enough with our nsIBrowserDOMWindow impls to do this?

Comment 11

[[e-mail]]

Distinguishing between null and an about:blank URI makes more sense to me; about:blank still works for anyone who needs the current window to be cleared.

Comment 12

[[e-mail]]

Attachment: What the suite patch might look like (untested)

Comment 13

[Damon Sicore (:damons)]

Scheduling for B2 per Boris.   Not going to land this, but Boris is going to redo the fix.

Comment 14

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Bz: feel free to reprioritize this. But this one does seem bad. Also let us know if we should try to find someone else to work on it.

Comment 15

[Boris Zbarsky [:bzbarsky]]

It's kinda bad... but only happens with non-default pref values that we don't expose in the UI (and imo shouldn't support at all, but whatever).

I can try to do it, but the patch will basically need to be in UI code, not in core Gecko, so I'll have to hunt the relevant code down, etc.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Attachment: Fix

Comment 17

[Boris Zbarsky [:bzbarsky]]

Attachment: Same as diff -w

Gavin, can you review the browser/ changes?  Neil, can you cover suite/ ?  Benjamin, everything else?

I did audit the callers, and no one in the tree except the treeowner is really passing null into this interface anyway.

Comment 18

[[e-mail]]

Comment on attachment 290791 [details] [diff] [review]
Same as diff -w

>   /**
>    * Load a URI
>-   * @param aURI the URI to open. null is allowed; it means about:blank.
>+
>+   * @param aURI the URI to open. null is allowed.  If null is passed in, no
>+   *             load will be done, though the window the load would have
>+   *             happened in will be returned.
Does this need a uuid change? ;-)

>       case nsIBrowserDOMWindow.OPEN_NEWWINDOW:
>+       var uri = aURI ? aURI.spec : "about:blank";
>         return window.openDialog(getBrowserURL(), "_blank", "all,dialog=no",
>                                  uri, null, referrer);
I know it's a -w diff but that indentation still looks wrong...
I don't know whether it's worthwhile inlining that variable.

>+          if (aURI) {
>+            gBrowser.loadURIWithFlags(aURI.spec, loadflags);
>+          }
File style is not to brace single lines. That said, I wonder why we don't try/catch this version.

Comment 19

[Boris Zbarsky [:bzbarsky]]

> Does this need a uuid change? ;-)

I wasn't sure.  I could go either way, depending on what Benjamin thinks.

> I know it's a -w diff but that indentation still looks wrong...

I think a tab snuck in.  I'll fix.

> File style is not to brace single lines.

Will fix.

No idea about the try/catch, just like no idea about the lack of referrer in the Firefox code.  I tried to not change any of the existing behavior when loading.

Comment 20

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 290791 [details] [diff] [review]
Same as diff -w

>Index: browser/base/content/browser.js

>     switch(aWhere) {
>       case Ci.nsIBrowserDOMWindow.OPEN_NEWWINDOW :
>+        // XXX so how come this doesn't send the referrer like the other loads
>+        // do?

This just looks like a bug. File it, and put the # in a FIXME comment?

Comment 21

[Benjamin Smedberg]

Comment on attachment 290791 [details] [diff] [review]
Same as diff -w

I think we should change the UUID so callers (even JS callers) can distinguish between the 1.8.1 and 1.9 behaviors.

Comment 22

[Boris Zbarsky [:bzbarsky]]

(In reply to comment #20)
> This just looks like a bug. File it, and put the # in a FIXME comment?

Filed bug 408379.

Addressed the rest of the comments.

Checked in.