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

Status: VERIFIED FIXED

(WebExtensions :: General, defect)

Description

[fiveNinePlusR]

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.

Comment 1

[fiveNinePlusR]

Attachment: Bug: - Fall back to the tab.label when there is no browser.contentTitle available

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.

Comment 2

[Kris Maglione [:kmag]]

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;
    }

Comment 3

[fiveNinePlusR]

Attachment: Update the patch based on Kris' feedback.

Comment 4

[fiveNinePlusR]

Attachment: Adds features required for testing and a testcase.

Comment 5

[Kris Maglione [:kmag]]

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.

Comment 6

[Kris Maglione [:kmag]]

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.

Comment 7

[fiveNinePlusR]

Attachment: bug1289213_patch1.diff

Comment 8

[fiveNinePlusR]

Attachment: bug1289213_patch2.diff

Comment 9

[Kris Maglione [:kmag]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3acbbebdd50e

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Those failures in try certainly look like they're the fault of these patches, have they been taken care of?

Comment 11

[fiveNinePlusR]

Attachment: bug1289213_patch2-2.diff

Should clear up the build breaking issues.

Comment 12

[Kris Maglione [:kmag]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=17c947998931

Comment 13

[fiveNinePlusR]

Attachment: bug1289213_patch2-3.diff

Comment 14

[Kris Maglione [:kmag]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5b181a4270ea

Comment 15

[Pulsebot]

Pushed by [e-mail]:
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

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/939f6cfffc28
https://hg.mozilla.org/mozilla-central/rev/9f83c03e8daf

Comment 17

[CosminB]

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.

[e-mail] 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 ?

Comment 18

[fiveNinePlusR]

CosminB, The patch I submitted fixed it for me. Should be fine moving forward.

Comment 19

[CosminB]

Based on Comment 18 this issue is verified as fixed.