The default bug view has changed. See this FAQ.

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

VERIFIED FIXED in Firefox 40

Status

()

Firefox
Bookmarks & History
--
critical
VERIFIED FIXED
3 years ago
2 years ago

People

(Reporter: marco349s, Assigned: billm)

Tracking

({dataloss, dogfood, regression})

35 Branch
Firefox 40
dataloss, dogfood, regression
Points:
---
Dependency tree / graph
Bug Flags:
firefox-backlog +
qe-verify +

Firefox Tracking Flags

(e10sm6+, firefox40 verified)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

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

3 years ago
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.
Blocks: 1051017
Status: UNCONFIRMED → NEW
tracking-e10s: --- → ?
Ever confirmed: true
Keywords: regression

Updated

3 years ago
Component: Untriaged → Bookmarks & History
likely something around this code
http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser-places.js#277

Comment 3

3 years ago
Also present under OSX (running 35.0a1 (2014-09-13))
(Assignee)

Comment 4

3 years ago
Sorry for all these regressions Alice. We're still very early in the process of getting automated testing working for e10s.
Assignee: nobody → wmccloskey
(Assignee)

Comment 5

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

Comment 6

3 years ago
Created attachment 8491200 [details] [diff] [review]
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.
Attachment #8491200 - Flags: review?(mak77)
Severity: normal → critical
Keywords: dataloss
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.
Attachment #8491200 - Flags: review?(mak77) → review-
tracking-e10s: ? → +
(Assignee)

Updated

3 years ago
Duplicate of this bug: 1073329
(Assignee)

Updated

3 years ago
Duplicate of this bug: 1082800
/me suprised this is not an m2 or m1 blocker
OS: Windows 8.1 → All
Hardware: x86_64 → All
Keywords: dogfood
Depends on: 1094205
Flags: qe-verify+
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?
(Assignee)

Updated

2 years ago
Duplicate of this bug: 1110156
tracking-e10s: + → ?
tracking-e10s: ? → m6+

Updated

2 years ago
Duplicate of this bug: 1111297

Updated

2 years ago
Duplicate of this bug: 1111882
Flags: firefox-backlog+

Updated

2 years ago
Duplicate of this bug: 1115550
Duplicate of this bug: 1122888
Duplicate of this bug: 1129468

Comment 18

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

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

Comment 20

2 years ago
(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

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

Disregard my previous comment.
(Assignee)

Comment 22

2 years ago
Created attachment 8585097 [details] [diff] [review]
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.
Attachment #8491200 - Attachment is obsolete: true
Attachment #8585097 - Flags: review?(mak77)
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.
Attachment #8585097 - Flags: review?(mak77) → review+
(Assignee)

Comment 24

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

Updated

2 years ago
Blocks: 1094205
No longer depends on: 1094205

Updated

2 years ago
Duplicate of this bug: 1149468
https://hg.mozilla.org/mozilla-central/rev/e0d49f2c364a
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox40: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 40

Updated

2 years ago
Iteration: --- → 40.1 - 13 Apr
QA Whiteboard: see also comment #11
QA Contact: andrei.vaida
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).
Status: RESOLVED → VERIFIED
status-firefox40: fixed → verified
You need to log in before you can comment on or make changes to this bug.