chrome.tabs does not retain the title when tabs are unloaded after a restart.

VERIFIED FIXED in Firefox 51

Status

defect
VERIFIED FIXED
3 years ago
Last year

People

(Reporter: fiveNinePlusR, Assigned: fiveNinePlusR, Mentored)

Tracking

49 Branch
mozilla51

Firefox Tracking Flags

(firefox51 verified)

Details

(Whiteboard: tabs, triaged)

Attachments

(2 attachments, 5 obsolete attachments)

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.11; rv:49.0) Gecko/20100101 Firefox/49.0
Build ID: 20160724004007

Steps to reproduce:

Wrote a webextension that loops through all the tabs using 

chrome.tabs.query({}, function(tabs){
  for(var i = 0, len = tabs.length; i < len; i++){
    console.log("tab: " + tabs[i].title + " - " + tabs[i].url);
}
}); 

and there are many tabs listed without a title even though the original page had a title. 


Actual results:

many undefined titles with urls that have titles in the console log. 


Expected results:

I'd expect there to be a retained title.
Component: Untriaged → WebExtensions: General
Product: Firefox → Toolkit
When a tab is unloaded, the session browser.contentTitle is unavailable so this patch allows the title to be exposed when the tab is unloaded.
Flags: needinfo?(kmaglione+bmo)
Flags: needinfo?(kmaglione+bmo)
Attachment #8776217 - Flags: review?(kmaglione+bmo)
Comment on attachment 8776217 [details] [diff] [review]
Bug:  - Fall back to the tab.label when there is no browser.contentTitle available

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

The change looks good. Ideally, we should add a test for this, but the only relatively simple way I can think of doing that is to duplicate the tab (which uses the session store to save the tab data and then restore it into a new tab). But that would require making a slight modification to the session store code to allow the tab to optionally be restored lazily:

http://searchfox.org/mozilla-central/rev/1112b7a5222b71a3b5b68bd531f50ded6bcbc770/browser/components/sessionstore/SessionStore.jsm#2091

Let me know if you want to take that on. Otherwise, I may consider allowing this to land without a test, but I'd really rather it not.

::: browser/components/extensions/ext-utils.js
@@ +414,5 @@
>      if (this.hasTabPermission(tab)) {
>        result.url = browser.currentURI.spec;
>        if (browser.contentTitle) {
>          result.title = browser.contentTitle;
> +      }else if (tab.label) {

Space after curly brace, please (ESLint will not accept this as-is).

Also, I think we'd generally write this as something more like:

    let title = browser.contentTitle || tab.label;
    if (title) {
      result.title = title;
    }
Attachment #8776217 - Flags: review?(kmaglione+bmo)
Assignee: nobody → mapish
Mentor: kmaglione+bmo
Status: UNCONFIRMED → NEW
Ever confirmed: true
Attachment #8776217 - Attachment is obsolete: true
Attachment #8776756 - Flags: review?(kmaglione+bmo)
Attachment #8776758 - Flags: review?(kmaglione+bmo)
Comment on attachment 8776758 [details] [diff] [review]
Adds features required for testing and a testcase.

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

Looks good. Thanks! Just some nits to address, and the test changes need to be moved to the other commit.

::: browser/components/extensions/test/browser/browser_ext_tabs_duplicate.js
@@ +39,5 @@
>      yield BrowserTestUtils.removeTab(gBrowser.tabs[0]);
>    }
>  });
>  
> +add_task(function* testDuplicateTabLazily() {

This test should be added as part of the commit that touches WebExtension code rather than the one that touches sessionstore code. Just be sure to add "Part 1" and "Part 2" to your commit messages so we can be sure they get landed in the right order.

@@ +40,5 @@
>    }
>  });
>  
> +add_task(function* testDuplicateTabLazily() {
> +

Our ESLint configuration doesn't accept blank lines at the start or end of a block. It'll also complain about the style issues I mention below.

@@ +54,5 @@
> +    let url = "http://example.com/browser/browser/components/extensions/test/browser/file_dummy.html";
> +    browser.tabs.create({url}, tab => {
> +      browser.test.sendMessage("duplicate-tab", tab.id);
> +
> +      tabLoadComplete.then( (unloadedTabId) => {

No space after opening `(`

@@ +56,5 @@
> +      browser.test.sendMessage("duplicate-tab", tab.id);
> +
> +      tabLoadComplete.then( (unloadedTabId) => {
> +
> +          browser.tabs.get(tab.id, function(loadedtab){

There's an extra two spaces of indentation. Please use arrow functions for these callbacks. Also, always include a space before the opening '{' of a block.

@@ +98,5 @@
> +  yield extension.unload();
> +
> +  while (gBrowser.tabs[0].linkedBrowser.currentURI.spec === "http://example.com/") {
> +    yield BrowserTestUtils.removeTab(gBrowser.tabs[0]);
> +  }

This loop isn't necessary, since this test isn't creating any tabs with this URL.
Attachment #8776758 - Flags: review?(kmaglione+bmo) → review+
Comment on attachment 8776756 [details] [diff] [review]
Update the patch based on Kris' feedback.

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

Looks good. Just need to move the tests from the session store patch either to this commit, or a separate commit that comes after it.
Attachment #8776756 - Flags: review?(kmaglione+bmo) → review+
Attachment #8776758 - Attachment is obsolete: true
Attachment #8777610 - Flags: review?(kmaglione+bmo)
Posted patch bug1289213_patch2.diff (obsolete) — Splinter Review
Attachment #8776756 - Attachment is obsolete: true
Attachment #8777611 - Flags: review?(kmaglione+bmo)
Attachment #8777610 - Flags: review?(kmaglione+bmo) → review+
Attachment #8777611 - Flags: review?(kmaglione+bmo) → review+
Keywords: checkin-needed
Whiteboard: tabs, triaged
Those failures in try certainly look like they're the fault of these patches, have they been taken care of?
Flags: needinfo?(kmaglione+bmo)
Flags: needinfo?(fiveNinePlusR)
Keywords: checkin-needed
Posted patch bug1289213_patch2-2.diff (obsolete) — Splinter Review
Should clear up the build breaking issues.
Attachment #8777611 - Attachment is obsolete: true
Flags: needinfo?(fiveNinePlusR)
Attachment #8780338 - Flags: review+
Attachment #8780338 - Attachment is obsolete: true
Attachment #8780362 - Flags: review+
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/939f6cfffc28
Part 1: Add ability to duplicate tabs lazily from the session store. r=kmag
https://hg.mozilla.org/integration/mozilla-inbound/rev/9f83c03e8daf
Part 2: Fall back to the tab.label when there is no browser.contentTitle available. r=kmag
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/939f6cfffc28
https://hg.mozilla.org/mozilla-central/rev/9f83c03e8daf
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
I was unable to reproduce this issue on Firefox 51.0a1 (2016-08-15) using Windows 7 64-bit and Firefox 50.0a1(2016-07-25) using Mac OS X 10.11.6. 

I also checked on Firefox 52.0a1 (2016-10-10) using Windows 7 and Firefox 51.0a2 (2016-10-11) using Mac OS X 10.11.6 but I did not see any differences.

fiveNinePlusR@gmail.com can you please confirm that this bug is fixed on http://archive.mozilla.org/pub/firefox/nightly/2016/10/2016-10-10-00-40-16-mozilla-aurora/firefox-51.0a2.en-US.mac.dmg ?
Flags: needinfo?(fiveNinePlusR)
CosminB, The patch I submitted fixed it for me. Should be fine moving forward.
Flags: needinfo?(fiveNinePlusR)
Based on Comment 18 this issue is verified as fixed.
Status: RESOLVED → VERIFIED
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.