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

RESOLVED FIXED

Status

()

--
minor
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: samuli.karkkainen, Assigned: justin.lebar+bug)

Tracking

Trunk
x86
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 4 obsolete attachments)

(Reporter)

Description

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

Updated

8 years ago
Component: Bookmarks & History → Document Navigation
Product: Firefox → Core
QA Contact: bookmarks → docshell
Version: unspecified → Trunk
> 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.
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?
(Reporter)

Comment 4

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

8 years ago
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".
> 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.
Status: UNCONFIRMED → NEW
Component: Document Navigation → Places
Ever confirmed: true
Product: Core → Toolkit
QA Contact: docshell → places
Summary: replaceState() causes empty window title to show in history → replaceState() causes empty window title to show in Firefox history menu (_not_ session history)
(Assignee)

Comment 7

8 years ago
I see this bug on Mac, trunk build.  bz, are you sure you cleared your history?
(Assignee)

Comment 8

8 years ago
Created attachment 530430 [details]
Screenshot of errant history menu on Mac
> bz, are you sure you cleared your history?

Pretty sure I used a clean profile... but who knows.
(Assignee)

Comment 10

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

Comment 11

8 years ago
Created attachment 530441 [details] [diff] [review]
Patch v1

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

Comment 12

8 years ago
Created attachment 530458 [details] [diff] [review]
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?
(Assignee)

Updated

8 years ago
Attachment #530441 - Attachment is obsolete: true
(Assignee)

Updated

8 years ago
Attachment #530458 - Flags: review?(bzbarsky)
(Assignee)

Comment 13

8 years ago
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.
Blocks: 500328
(Assignee)

Comment 14

8 years ago
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!

Updated

8 years ago
Assignee: nobody → justin.lebar+bug
(Assignee)

Comment 15

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

Comment 17

8 years ago
Created attachment 531186 [details] [diff] [review]
Patch v2 with Chrome mochitest

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

Updated

8 years ago
Attachment #530458 - Attachment is obsolete: true
Attachment #530458 - Flags: review?(bzbarsky)
(Assignee)

Updated

8 years ago
Attachment #530458 - Attachment is obsolete: false
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?
(Assignee)

Comment 19

8 years ago
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?
(Assignee)

Comment 20

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

Comment 21

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

Comment 22

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

Comment 23

8 years ago
Created attachment 531388 [details] [diff] [review]
Patch v2.1

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

Updated

8 years ago
Attachment #530458 - Attachment is obsolete: true
(Assignee)

Updated

8 years ago
Attachment #531186 - Attachment is obsolete: true
(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.
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?
(Assignee)

Comment 26

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

Comment 27

8 years ago
Created attachment 531415 [details] [diff] [review]
Patch v3

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

Updated

8 years ago
Attachment #531388 - Attachment is obsolete: true
(Assignee)

Updated

8 years ago
Attachment #531415 - Flags: review?(mak77)
I'll look at this soon, b-c test sounds like a really good idea and good to know about content docshell.
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.
Attachment #531415 - Flags: review?(mak77) → review+
(Assignee)

Comment 30

8 years ago
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.)
(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.
(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? :)
(Assignee)

Comment 33

8 years ago
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.  :)
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!
(Assignee)

Comment 35

8 years ago
http://hg.mozilla.org/mozilla-central/rev/834c11d2a8ac
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.