1067042 - [e10s]bookmarking an e10s tab results in a blank name

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[marco349s]

User Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:35.0) Gecko/20100101 Firefox/35.0
Build ID: 20140913030206

Steps to reproduce:

bookmarking a newly opened e10s tab (either with ctrl+D or with the star button) results in a blank bookmark name


Actual results:

the newly created bookmark had no name


Expected results:

the newly created bookmark should have the tab's title as name (unless changed)

Comment 1

[Alice0775 White]

Regression window(m-i)
Good:
https://hg.mozilla.org/integration/mozilla-inbound/rev/72f6354159d5
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:34.0) Gecko/20100101 Firefox/34.0 ID:20140820124716
Bad:
https://hg.mozilla.org/integration/mozilla-inbound/rev/304619045d61
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:34.0) Gecko/20100101 Firefox/34.0 ID:20140820125117
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=72f6354159d5&tochange=304619045d61


Regressed by
	d6bdd3272ccb	Bill McCloskey — Bug 1051017 - Add browser.contentWindowAsCPOW and browser.contentDocumentAsCPOW (r=mconley,mrbkap)


I wonder there is no automation test.

Comment 2

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

likely something around this code
http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser-places.js#277

Comment 3

[dch]

Also present under OSX (running 35.0a1 (2014-09-13))

Comment 4

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Sorry for all these regressions Alice. We're still very early in the process of getting automated testing working for e10s.

Comment 5

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

This line doesn't look too good:
http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser-places.js#315
contentWindow will be null after the regressing patch.

Comment 6

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: bookmark-fix

This code is a little tricky because of bug 52536. I don't know what a "fullpage plugin" is. However, I think nothing should be broken. The last sentence of bug 52536 comment 0 suggests that fullpage plugins don't have a document. Therefore, I don't think that webNav.document.title is any better than aBrowser.contentTitle. Same for the charset.

Eventually we'll want to get rid of the CPOW for the description, but it doesn't seem too important right now.

Comment 7

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 8491200 [details] [diff] [review]
bookmark-fix

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

::: browser/base/content/browser-places.js
@@ +286,5 @@
>        var description;
>        var charset;
>        try {
>          let isErrorPage = /^about:(neterror|certerror|blocked)/
> +                          .test(webNav.currentURI);

unfortunately this is not going to work, if i open an error page, by visting for example http://www.zzyzz.com/ (that doesn't exist)
and I print out webNavigation.currentURI, it's "http://www.zzyzz.com/

but webNavigation.document.documentURI is about:neterror?e=dnsNotFound&u=http%3A//www.zzyzz.com/&c=UTF-8&f=regular&d=Firefox%20can%27t%20find%20the%20server%20at%20www.zzyzz.com.

clearly in this case we care about the fact it's an error page and the former wouldn't tell us that. So we need an alternative way to figure it's an error/cert/blocked page

@@ +291,2 @@
>          title = isErrorPage ? PlacesUtils.history.getPageTitle(url)
> +                            : aBrowser.contentTitle;

If we remove the workaround for "full page plug-ins" we should just remove the above comment about them.

Fwiw, the current code is indeed already broken. You can try that setting Adobe Acrobat (Nightly) as the default pdf reader (yes, that's this exotic full page plugin thing). Then open a pdf and try to get the title with the new or old method... both are failing and getting an empty string

I think we need a bug filed about this issue and replace the comment above (that regardless is wrong) with a
// TODO (bug 123456): Fix full page plugins (like adobe reader) titles.
or whatever comment you find appropriate here

it's not extremely important that we fix that here, full page plugins are not that common, let alone bookmark them.

Comment 10

[Wayne Mery (:wsmwk)]

/me suprised this is not an m2 or m1 blocker

Comment 11

[Camelia Badau [:cbadau], Desktop QA]

Also reproducing if I bookmark a newly opened e10s tab (with the star button) and I select Folder: Bookmarks Toolbar -> The bookmarked website is displayed inside the toolbar, but the title is blank; only the icon of the websites is displayed.

Does this bug also cover the mentioned scenario or I should file a new bug for it?

Comment 18

[leichixian]

Just a reminder: It's near Mar 31th, which is the ending of Q1 2015, and ending of M6. Any updates on this bug?

Comment 19

[pedrogfrancisco]

The bookmark part I think it's fixed on my Nightly.

Comment 20

[leichixian]

(In reply to pedrogfrancisco from comment #19)
> The bookmark part I think it's fixed on my Nightly.

Really? Did you try adding a bookmark under e10s mode? Here when I try to add a bookmark under e10s, I can't get the tab title. I'm using NIghtly (build 20150326030312 x64 zh-CN localized version) on windows 7

Comment 21

[pedrogfrancisco]

Then disregard my comment. The a11y thing (I have a touchscreen) is always disabling e10s without me noticing it.

Disregard my previous comment.

Comment 22

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: bookmark-fix 2

Well, this uses more CPOWs, but at least it's correct. When we fix the CPOW for the description, we can fix the documentURI one too.

Comment 23

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 8585097 [details] [diff] [review]
bookmark-fix 2

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

::: browser/base/content/browser-places.js
@@ +278,2 @@
>        var webNav = aBrowser.webNavigation;
>        var url = webNav.currentURI;

At this point, you can remove webNav and url, and just use uri (that is already defined)
We'll evaluate what to do in future for Bug 1148838.

@@ +288,1 @@
>          title = title || url.spec;

As I said, just use uri in getPageTitle and this title assignment.

Comment 24

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e0d49f2c364a

Comment 26

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e0d49f2c364a

Comment 27

[Andrei Vaida [:avaida]]

Verified fixed on Nightly 40.0a1 (2015-04-19), using Ubuntu 14.04 (x64), Mac OS X 10.9.5 and Windows 8.1 (x64).