1174779 - "Bookmark All Tabs" doesn't bookmark tabs that were not loaded (titled New Tab)

Status: NEW

(Firefox :: Bookmarks & History, defect, P5)

Description

[nivtwig]

User Agent: Mozilla/5.0 (Windows NT 6.1; rv:41.0) Gecko/20100101 Firefox/41.0
Build ID: 20150610030209

Steps to reproduce:

Starting Firefox with many tabs from the previous session, with the option "don't load tabs until selected" checked. 
( Some of them have the "New Tab" title because they were not selected yet, although when clicked they will load and change their title to the page title. )

Right click over the current tab, and choose the popup menu option "Bookmark All Tabs".

 


Actual results:

Many of the tabs were not saved/bookmarked to the folder created by "Bookmark All Tabs". 

These tabs are tabs from the previous session that have their title displayed as "New Tab" although those are actual tabs, and the only reason they have "New Tab" displayed on them is because they were not loaded until selected with the mouse.

When I click on some of them, they get loaded and their title changes from "New Tab" to the actual page title. An important note: Not all tabs that were not selected and loaded yet are marked with the title "New Tab", some of them have the actual title.

Therefore I suspect that these are tabs that were previously bookmarked with the "bookmark All Tabs" command when they were not loaded yet or during Firefox startup and load, and they were bookmarked by Firefox with the title "New Tab". Then, the bookmarks were opened with the "Open All in tabs" command on the bookmark folder, and some of them had the title "New Tab", and then when you close and reopen Firefox some of them still have the "New Tab" title (those that were not selected and loaded), and are not omitted and not bookmarked when you choose "Bookmark all tabs".



Expected results:

All tabs should have been saved/bookmarked including those titled "New Tab"

Comment 1

[nivtwig]

The bug is still present in the latest Nightly 41.0a1 (2015-06-15).
I changed (or tried to change) the severity to "Critical" since it causes loss of data without the user noticing it (especially when he has many tabs open and some of them are not bookmarked)

Comment 2

[Marco Bonardo [:mak]]

I think something changed recently in unloaded tabs? But maybe I'm wrong...

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

Can you find a reliable way to reproduce this? I'm not even sure what's going wrong because I don't understand the bug probably.

(In reply to nivtwig from comment #0)
> These tabs are tabs from the previous session that have their title
> displayed as "New Tab" although those are actual tabs, and the only reason
> they have "New Tab" displayed on them is because they were not loaded until
> selected with the mouse.

That's wrong. Pending tabs never should have "New Tab" as the title and I don't experience this issue.

Do you have any add-ons installed? Can you try to disable them one by one and see if you can still reproduce it?

Comment 4

[nivtwig]

(In reply to Tim Taubert [:ttaubert] (gone June 17-22, back in time for Whistler) from comment #3)
> Can you find a reliable way to reproduce this? I'm not even sure what's
> going wrong because I don't understand the bug probably.
> 
> (In reply to nivtwig from comment #0)
> > These tabs are tabs from the previous session that have their title
> > displayed as "New Tab" although those are actual tabs, and the only reason
> > they have "New Tab" displayed on them is because they were not loaded until
> > selected with the mouse.
> 
> That's wrong. Pending tabs never should have "New Tab" as the title and I
> don't experience this issue.
> 

Maybe its wrong, but this is fact. As I said in my previous comment, I have many tabs with "New Tab" as the title, and many other tabs with proper titles, and both 2 kinds of tabs are not loaded because they were not selected. 

I don't know how I got the tabs with "New Tab" title on them, therefore I currently can't reproduce the bug until I find out how I got them . I suggested a possible scenario how I got them in a previous comment but I can't check this because I will have to close my current tabs to test this, but I don't want to lose my current list of hundreds of tabs since they cannot be bookmarked correctly because of this bug...

But the fact is that the "New Tab" tabs exist. If you say they shouldn't exist, then maybe that's the problem...

The question is, regardless if "New Tab" titled tabs should exist or not, lets assume for the sake of argument that they exist, are they bookmarked or there is some known logic in the code that says they shouldn't and omits them?

> Do you have any add-ons installed? Can you try to disable them one by one
> and see if you can still reproduce it?

I still have the problem after restarting Firefox with add-ons disabled (on both stable 38.0.5 , and nightly builds)

Comment 5

[nivtwig]

Ok, I found a way to reproduce this.
I tested on a new profile but you don't have to create a new profile :

1. Create a bookmarks folder with about at least 10 different bookmarks, if you don't already have such a bookmarks folder. Lets call this folder "test1"

2. Session restore should be enabled. In the Firefox options, the option "When Firefox starts" should be set to "Show my windows and tabs from last time"

3. Close all tabs but the last one. 

4. Go to the "test1" bookmarks folder in the bookmark menu, right click and choose "Open All in Tabs", and quickly close Firefox after you see all the tab handles on the tab bar, but before most of them are loaded (I see "Connecting ..." on some of the tab titles)

5. Start firefox after it was closed (the option don't load tabs until selected should be active). 
   Now you should see many unloaded tabs with a "New Tab" title on their tab handles contrary to your assumption (that Pending tabs never should have "New Tab" as the title )

6. Again "Bookmark all tabs" to create a bookmarks folder "test2"

7. You should see that not all tabs were bookmarked in "test2". The tabs with "New Tab" title were not bookmarked .

Comment 6

[Hossain Al Ikram [:ikram] (QA Contact)]

I have reproduced this bug with the STR of Comment 5 in Firefox 39 Beta 7 (Build Id - 20150618135210 ) and on Windows 7, 64 Bit!

But for reproducing this bug,In step 4, you need to have some tabs which are still loading. If all the tabs are loaded, then you can not reproduce this bug!

Comment 7

[nivtwig]

Hossain, thank you very much.
Since you could reproduce this, can you please change the status of this bug from UNCONFIRMED to NEW ?
(I cannot do this since I am the reporter ...)

Comment 8

[nivtwig]

I found the buggy code :

When bookmarking all tabs, in browser.js file, the function bookmarkCurrentPages calls uniqueCurrentPages in order to bookmark the tabs without duplicates. 
The uniqueCurrentPages function looks at the tab's spec (tab.linkedBrowser.currentURI.spec) to determine if it was already in the list to prevent duplicates, but the spec is always "about:blank" for tabs titled in the browser UI with "New Tab", so it treats all of those tabs as duplicates (except the first one, I assume...), and doesn't save them in the list of pages sent by bookmarkCurrentPages to the bookmark all tabs dialog , although it should .

from browser.js :
  /**
   * List of nsIURI objects characterizing the tabs currently open in the
   * browser, modulo pinned tabs.  The URIs will be in the order in which their
   * corresponding tabs appeared and duplicates are discarded.
   */
  get uniqueCurrentPages() {
    let uniquePages = {};
    let URIs = [];
    gBrowser.visibleTabs.forEach(function (tab) {
      let spec = tab.linkedBrowser.currentURI.spec;
      if (!tab.pinned && !(spec in uniquePages)) {
        uniquePages[spec] = null;
        URIs.push(tab.linkedBrowser.currentURI);
      }
    });
    return URIs;
  },

  /**
   * Adds a folder with bookmarks to all of the currently open tabs in this 
   * window.
   */
  bookmarkCurrentPages: function PCH_bookmarkCurrentPages() {
    let pages = this.uniqueCurrentPages;
    if (pages.length > 1) {
    PlacesUIUtils.showBookmarkDialog({ action: "add"
                                     , type: "folder"
                                     , URIList: pages
                                     , hiddenRows: [ "description" ]
                                     }, window);
    }
  },

Comment 9

[nivtwig]

(In reply to Hossain Al Ikram (QA Contact) from comment #6)
> I have reproduced this bug with the STR of Comment 5 in Firefox 39 Beta 7
> (Build Id - 20150618135210 ) and on Windows 7, 64 Bit!

Anyone who has the permission to change this bug status from UNCONFIRMED to NEW, please do so since it was reproduced according to comment #6

Comment 10

[nivtwig]

A small correction to comment #8, the problematic javascript code is in browser/base/content/browser-places.js, and not in broswer.js. 

broswer.js includes browser-places.js , so it appeared in the broswer javacript debugger in broswer.js , although it is actually in browser-places.js .

Comment 11

[Moin Shaikh]

QA Whiteboard:[bugday-20150624]:

I tried to reproduced this based on Nivtwig's comment#5  but was unable to restore session after reopening Firefox on the step no.5 even when I have setting that will reopen browser session restore window. The restore window does not appear and a homepage opens as in a regular case. Not sure if this another bug or something.

Firefox 39 buil 7, Windows 7 (64bit)

Comment 12

[nivtwig]

(In reply to montugshaikh from comment #11)
> QA Whiteboard:[bugday-20150624]:
> 
> I tried to reproduced this based on Nivtwig's comment#5  but was unable to
> restore session after reopening Firefox on the step no.5 even when I have
> setting that will reopen browser session restore window. The restore window
> does not appear and a homepage opens as in a regular case. Not sure if this
> another bug or something.
> 
> Firefox 39 buil 7, Windows 7 (64bit)

This might happen when you close Firefox and the Firefox window closes, but the previous Firefox process didn't finish and takes a lone time to close (there are bugs about it)
In this case opening Firefox might not open a new Firefox process, but a new window for the old Firefox process (and stop the process from being close, since a new window was opened for it).

Comment 13

[nivtwig]

*) If someone has permissions, please confirm the bug (move to NEW status) as it was confirmed by QA person in comment #6, to expedite the chance that it gets fixed

*) Changing the Firefox version from Branch 38 to 39, in the hope that it will expedite fixing it before the upcoming Firefox 39 is released.

*) Some typo corrections for comment #12 : 
lone time => long time
being close => being closed

Comment 14

[nivtwig]

The bug was reproduced according to comment #6 , then why isn't the it confirmed (changed to status NEW) for several weeks ? 
Who has the permission to confirm and why isn't it confirmed ?

Please confirm it.

Comment 15

[nivtwig]

In my opinion, the bug severity should be changed to "Critical" since it causes loss of data, usually without the user even noticing it (especially when there are many tabs open and some of them are not bookmarked)

According to definition of Severity in https://bugzilla.mozilla.org/page.cgi?id=fields.html :
critical 	crashes, loss of data, severe memory leak

Comment 16

[nivtwig]

I don't see any progress on this critical bug, although I have found and pointed you to the problematic source code, in commment #8 .

Is there any update on this, is there anybody working or assigned to work on this bug?

Comment 17

[Marco Bonardo [:mak]]

Sorry but we have no resources to work on this at the moment. Most of the Places maintenance right now needs community help. So if you or someone else here wishes to contribute a patch, I'll gladly help reviewing it.
The unfortunate part is that it will need an automated mochitest-browser test that may not be trivial to write (I don't know much about the session restore testing support, but there are experts like ttaubert or dteller to ask to).

fwiw, critical is only used for crash bugs, but regardless nobody is using those flags for querying bugs, so setting them is not going to give this any more priority.

Comment 18

[Mark Banner (:standard8)]

This my have been helped or fixed by bug 610357. First steps would be to try reproducing again and see what issues arise.