646422 - replaceState() causes empty window title to show in Firefox history menu (_not_ session history)

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[Samuli Kärkkäinen]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2.16) Gecko/20110319 Firefox/3.6.16
Build Identifier: 4.x

Assume a web page with following content:

<title>sometitle</title>
<a onclick="history.replaceState({}, '', 'newurl'); return true;" href="#2">clickme</a>

When I go to that page, I have one history entry, with title "sometitle". When I click on the link, I get two more entries, of which the first one shows the URL as the title in the History menu. When I click the browser back button, the title becomes correct in the History menu.

According to all documentation I've found, such as https://developer.mozilla.org/en/DOM/Manipulating_the_browser_history , the second parameter to replaceState() should be ignored by Firefox. It appears not to be. If I give some other value than "", what ends up showing in the History menu is title of a page I've shown in the tab in past. Again when I use the back button the title becomes correct.

Also, shouldn't replaceState() replace the existing history menu entry, rather than create a new one? Internally it might, because using the back button it's not possible to get to the original History menu entry.

Happens with 4.0 and 4.2a1pre nightly 2011-03-30 on 32 bit Linux. 


Reproducible: Always

Comment 1

[Boris Zbarsky [:bzbarsky]]

> When I click on the link, I get two more entries

I don't see this behavior with a current Mac nightly.  When I click the link in a testcase as described in comment 0, I get one new entry, and it has the title "sometitle".

> the second parameter to replaceState() should be ignored by Firefox

It is.  If you look at http://hg.mozilla.org/mozilla-central/file/06599b96dfd9/docshell/base/nsDocShell.cpp#l9595 the aTitle argument is never used.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: The testcase

Comment 3

[Boris Zbarsky [:bzbarsky]]

I don't see the reported issue with a Linux nightly either.

Reporter, are you testing this in safe mode, or at least in a profile with no extensions?

Comment 4

[Samuli Kärkkäinen]

Happens also in the safe mode.

By History menu I mean menu in the menu bar. Perhaps you're looking at the right mouse button menu on the back button instead? Its contents are correct.

Comment 5

[Jason Tackaberry]

I can confirm this happens on both Linux and Windows (Firefox 4 final).

1. Clear history (it's just easier to get the cruft out of the way).
2. Open test case attachment in new tab. 
3. Click "clickme" link in testcase

After step 2, you will see 1 entry, "sometitle", in the History menu as expected.

After step 3, you see 3 entries, "sometitle", "bug646422.bugzilla.mozilla.org/.../newurl" and "sometitle".

Comment 6

[Boris Zbarsky [:bzbarsky]]

> Perhaps you're looking at the right mouse button menu on the back button
> instead?

Ah, I was, yes.

Looking at the browser history menu, I do see the behavior you describe, but only on Linux.  On Mac it says "sometitle".

Also, that menu is showing things you have visited, period, not your back/forward (session) history entries, so replacing session history entries doesn't affect it.  For example, that menu shows things you might have visited before the last time you started the browser that are not associated with any open tabs....

Over to the right component for this.  The difference between Linux and Mac here is ... confusing.

Comment 7

[Justin Lebar (not reading bugmail)]

I see this bug on Mac, trunk build.  bz, are you sure you cleared your history?

Comment 8

[Justin Lebar (not reading bugmail)]

Attachment: Screenshot of errant history menu on Mac

Comment 9

[Boris Zbarsky [:bzbarsky]]

> bz, are you sure you cleared your history?

Pretty sure I used a clean profile... but who knows.

Comment 10

[Justin Lebar (not reading bugmail)]

The entry with the missing title is coming from the IHistory::VisitURI call explicitly triggered by nsDocShell::AddState [1].  (That entry disappears when I remove the call in docshell.)

It looks like the problem is that we never call SetURITitle on that URI.

[1] http://hg.mozilla.org/mozilla-central/file/88fdbd974f82/docshell/base/nsDocShell.cpp#l9731

Comment 11

[Justin Lebar (not reading bugmail)]

Attachment: Patch v1

This seems to fix things.  I wonder how hard it would be to write a test.

Comment 12

[Justin Lebar (not reading bugmail)]

Attachment: Patch v2

Okay, wrote a test.  I'm not sure if I'm doing the SpecialPowers business right; seems kind of silly that I have to call into it just to run nsURI::GetSpec().

bz, do you want to review?

Comment 13

[Justin Lebar (not reading bugmail)]

Turns out that the back button drop-down-list is also pretty messed up, although this time due to pushState.  And favicons don't work properly either.

Comment 14

[Justin Lebar (not reading bugmail)]

If you're following along at home, broken favicons are bug 655270, and the fact that the drop-down list on the back button doesn't have title is bug 655273.

Thanks for filing this bug and getting me to look at this, Samuli!

Comment 15

[Justin Lebar (not reading bugmail)]

(In reply to comment #12)
> Okay, wrote a test.  I'm not sure if I'm doing the SpecialPowers business
> right; seems kind of silly that I have to call into it just to run
> nsURI::GetSpec().

I tried modifying this test so it runs as a chrome mochitest, but the titlechange observer doesn't fire for some reason.  We may be stuck with this special powers business.

Comment 16

[Shawn Wilsher :sdwilsh]

(In reply to comment #15)
> I tried modifying this test so it runs as a chrome mochitest, but the
> titlechange observer doesn't fire for some reason.  We may be stuck with this
> special powers business.
That should work in a chrome mochitest.  Care to post that test file?

Comment 17

[Justin Lebar (not reading bugmail)]

Attachment: Patch v2 with Chrome mochitest

It doesn't appear that the onTitleChange observer ever gets fired in this Chrome test.

Comment 18

[Marco Bonardo [:mak]]

this test works: http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/tests/browser/browser_settitle.js
and is using setURITitle and onTitleChanged

The things I see here (but I don't think they are related), observer is missing QueryInterface and the test is html rather than xul.
where is this "Title after pushstate." title setup? are we sure we actually have any title set here?

Comment 19

[Justin Lebar (not reading bugmail)]

I have another html Chrome test which works fine.  Maybe I do need the QI, although it's weird that it works without it in the non-chrome Mochitest.

> where is this "Title after pushstate." title setup? are we sure we actually 
> have any title set here?

The page has a <title>, although maybe that's ignored in the Chrome test?

Comment 20

[Justin Lebar (not reading bugmail)]

Adding the QI as in browser_settitle.js didn't help.  And I verified that document.title does contain the page's <title>.

Maybe the failure has to do with this:

WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80004005: file ../../../../src/toolkit/components/places/nsNavBookmarks.cpp, line 2901

looking into it...

Comment 21

[Justin Lebar (not reading bugmail)]

bz says this isn't going to work as a chrome mochitest in its current form:

bz: you want to create a content docshell
bz: in your chrome
bz: then load stuff in that
bz: but have all your test script in the chrome
bz: you may need to open a separate chrome window
bz: to hold the content iframe
bz: because your chrome mochitests don't run in chrome docshells

Comment 22

[Justin Lebar (not reading bugmail)]

Ted, would you mind if I added the following functions to SpecialPowers:

 _getNavHistoryService()
 addNavHistoryObserver()
 removeNavHistoryObserver()
 getURISpec()

?  That sounds much simpler than writing a Chrome test, and it looks like there are already plenty of ad-hoc functions in specialpowers.js.

Comment 23

[Justin Lebar (not reading bugmail)]

Attachment: Patch v2.1

Cleaning up a bit of SpecialPowers.  Using SpecialPowers plus a plain mochitest seems like the more sane way to go.

Comment 24

[Shawn Wilsher :sdwilsh]

(In reply to comment #22)
> ?  That sounds much simpler than writing a Chrome test, and it looks like there
> are already plenty of ad-hoc functions in specialpowers.js.
You can avoid the special powers stuff by writing a browser chrome tests.  Just open a new tab (gBrowser.newTab) and you instantly get a content docshell.

Comment 25

[(not currently active) Ted Mielczarek]

It's certainly kind of fuzzy. My general thoughts are:
* If you're testing web content, you should probably write a Mochitest
* If you're testing Gecko internals, you should probably write a chrome Mochitest
* If you're testing the Browser UI, you definitely want to write a browser chrome test

This case is...not so clear to me, but this is a bug in a web-facing feature that impacts the Firefox UI, right?

Comment 26

[Justin Lebar (not reading bugmail)]

> this is a bug in a web-facing feature that impacts the Firefox UI, right?

Yes.

I don't really care how we do it at this point; I just want to be done with this test.  :)

Comment 27

[Justin Lebar (not reading bugmail)]

Attachment: Patch v3

Good call on using a browser-chrome test.  It works great.  Thanks!

Comment 28

[Marco Bonardo [:mak]]

I'll look at this soon, b-c test sounds like a really good idea and good to know about content docshell.

Comment 29

[Marco Bonardo [:mak]]

Comment on attachment 531415 [details] [diff] [review]
Patch v3

Review of attachment 531415 [details] [diff] [review]:
-----------------------------------------------------------------

Just as a safety check, do a mochitest-oth run on tryserver, to avoid eventual random failures we could have missed.

::: docshell/base/nsDocShell.cpp
@@ +9678,5 @@
>  
>          AddURIVisit(newURI, oldURI, oldURI, 0);
> +
> +        // AddURIVisit doesn't set the title for the new URI in global history,
> +        // so do that here (bug 646422).

nit: I don't think annotating fixed bugs numbers is really useful, blame exists for that scope. I'd just say "// Set the title for the new URI in global history."

::: toolkit/components/places/tests/browser/browser_bug646422.js
@@ +32,5 @@
> + * and other provisions required by the GPL or the LGPL. If you do not delete
> + * the provisions above, a recipient may use your version of this file under
> + * the terms of any one of the MPL, the GPL or the LGPL.
> + * 
> + * ***** END LICENSE BLOCK ***** */

nit: if you wish, you can use a public domain license header in tests (http://www.mozilla.org/MPL/boilerplate-1.1/pd-c) that is much shorter

@@ +43,5 @@
> +const Cc = Components.classes;
> +const Ci = Components.interfaces;
> +
> +let navHistory = Cc["@mozilla.org/browser/nav-history-service;1"]
> +                   .getService(Ci.nsINavHistoryService);

no need for const Cc, Ci, browser has those

Also, directly use PlacesUtils.history in the test to refer to the history service. it's already available in browser scope

@@ +55,5 @@
> +  tabBrowser.addEventListener('load', function(aEvent) {
> +    tabBrowser.removeEventListener('load', arguments.callee, true);
> +
> +    // Control should now flow down to observer.onTitleChanged().
> +    let cw = tab.linkedBrowser.contentWindow;

you added a tabBrowser shortcut before for tab.linkedBrowser

@@ +60,5 @@
> +    ok(cw.document.title, 'Content window should initially have a title.');
> +    cw.history.pushState('', '', 'new_page');
> +  }, true);
> +
> +  let observer = {

please add QueryInterface: XPCOMUtils.generateQI([Ci.nsINavHistoryObserver]) even if it can work fine without it

@@ +65,5 @@
> +    onTitleChanged: function(uri, title) {
> +      // If the uri of the page whose title is changing ends with 'new_page',
> +      // then it's the result of our pushState.
> +      if (/new_page$/.test(uri.spec)) {
> +        is(title, tab.linkedBrowser.contentWindow.document.title,

ditto for tabBrowser shortcut

@@ +85,5 @@
> +  };
> +
> +  // The |false| argument here indicates that the nav history should keep a
> +  // strong ref to the observer.  Thus we need to remove the observer when
> +  // we're done with it!

nit: Unless you find this really useful, I'd remove this comment, the idl is there to check for whoever has doubts on what the second argument is.

Comment 30

[Justin Lebar (not reading bugmail)]

Thanks for the review!

> please add QueryInterface: XPCOMUtils.generateQI([Ci.nsINavHistoryObserver]) 
> even if it can work fine without it

Out of curiosity, why?  Might this break at some later point in time?

> nit: Unless you find this really useful, I'd remove this comment, the idl is 
> there to check for whoever has doubts on what the second argument is.

I have a personal vendetta against public functions which take opaque boolean arguments.  IMO it should be two functions, say AddWeakObserver and AddStrongObserver, specifically because I think it's a waste of time to look up the IDL every time you encounter a function like this.  (This case is particularly bad, because even if I remember that the boolean has to do with strong/weak references, I'm not going to remember whether false is strong or weak.)

Comment 31

[Shawn Wilsher :sdwilsh]

(In reply to comment #30)
> Out of curiosity, why?  Might this break at some later point in time?
xpconnect will always just say "yes, this QIs to what you asked about", whereas if you actually specify it, it only gives out that type.

> I have a personal vendetta against public functions which take opaque boolean
> arguments.  IMO it should be two functions, say AddWeakObserver and
> AddStrongObserver, specifically because I think it's a waste of time to look up
> the IDL every time you encounter a function like this.  (This case is
> particularly bad, because even if I remember that the boolean has to do with
> strong/weak references, I'm not going to remember whether false is strong or
> weak.)
But this is the same signature as what nsIObserverService uses, and that's used throughout the tree.  You have to know how that works if you want to stay sane, sadly.

Comment 32

[Marco Bonardo [:mak]]

(In reply to comment #30)
> Thanks for the review!
> 
> > please add QueryInterface: XPCOMUtils.generateQI([Ci.nsINavHistoryObserver]) 
> > even if it can work fine without it
> 
> Out of curiosity, why?  Might this break at some later point in time?

A lot of ext-devs may use tests as code examples, thus I like us showing good coding practices.

> I have a personal vendetta against public functions which take opaque
> boolean arguments.  IMO it should be two functions, say AddWeakObserver and
> AddStrongObserver, specifically because I think it's a waste of time to look
> up the IDL every time you encounter a function like this.

Personally I'd just kill the weak possiblity, it's unused internally. But adding a long comment to each usage is not going to help that. File a bug instead? :)

Comment 33

[Justin Lebar (not reading bugmail)]

I think there's some tension between "use good practices to help new people who may read your code" and "don't add this comment, since everyone who reads your code will understand."

But anyway, I'll stop arguing.  :)

Comment 34

[Marco Bonardo [:mak]]

yes, I was saying "everyone who reads your code should find a good example, and then use the documentation to figure out what he's missing".
I'm still learning myself everyday!

Comment 35

[Justin Lebar (not reading bugmail)]

http://hg.mozilla.org/mozilla-central/rev/834c11d2a8ac