1385733 - "bookmark all tabs" doesn't save most tabs from 2017-07-26 nightly or earlier

Status: VERIFIED FIXED

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

Description

[nivtwig]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:56.0) Gecko/20100101 Firefox/56.0
Build ID: 20170730100307

Steps to reproduce:

I started Firefox Nightly with many tabs restored from the previous session, about 880 tabs. Right clicked over the first tab, and clicked "Bookmark All Tabs"



Actual results:

The dialog appears without the usual "[Folder Name]" and Folder (the parent folder) input fields, therefore I cannot type a folder name.
Only the "Add Bookmarks" and "Cancel" buttons appear.
Since I couldn't type a folder name to use, I clicked the "Add Bookmarks" button, without giving a folder name.

In Nightly 2017-07-26, a default bookmarks folder with the name "[Folder Name]" was created, but was empty (instead of with about 880 tabs ...)

After updating to the latest Nightly 2017-07-30, a default bookmarks folder with the name "[Folder Name]" was created, is not empty like with Nightly 2017-07-26, but has only 76 tabs, most of the tabs are missing  (instead of about 880 tabs ...)

After some tries, I found out later that if you wait long enough (about 30 seconds) after clicking "Bookmark all tabs", the missing 2 input fields folder name and Folder (parent folder), suddenly appear (!) in the dialog box, enlarging it, and giving me the option to type the folder name. When this happens, all tabs seem to be bookmarked correctly, without missing tabs. 

The time it takes for the missing input fields to appear, sometimes depend on the number of tabs you have in the window, so if you bookmark all tabs on windows with a small number of tabs, you might not notice this.




Expected results:

The input fields on the "bookmark all tabs" dialog box should have appeared immediately. 

When bookmarking all tabs without entering a folder name (with the default folder name) before those input fields suddenly appear, all tabs should be bookmarked and there should not be many missing tabs that are not bookmarked.

Comment 1

[nivtwig]

might be related to Bug 1385737 : 
Cannot delete bookmarks from Nightly 2017-07-30 or earlier

Comment 2

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

(In reply to nivtwig from comment #0)
> The dialog appears without the usual "[Folder Name]" and Folder (the parent
> folder) input fields, therefore I cannot type a folder name.
> Only the "Add Bookmarks" and "Cancel" buttons appear.

This is bug 1383138 that is fixed from Nightly 27.

> After updating to the latest Nightly 2017-07-30, a default bookmarks folder
> with the name "[Folder Name]" was created, is not empty like with Nightly
> 2017-07-26, but has only 76 tabs, most of the tabs are missing  (instead of
> about 880 tabs ...)

Any errors reported to the browser console?

> After some tries, I found out later that if you wait long enough (about 30
> seconds) after clicking "Bookmark all tabs", the missing 2 input fields
> folder name and Folder (parent folder), suddenly appear (!) in the dialog
> box, enlarging it, and giving me the option to type the folder name. When
> this happens, all tabs seem to be bookmarked correctly, without missing
> tabs. 

I think the problem is that we must collect information about all the tabs and create all the bookmarks before proceeding. 880 tabs is a lot to do that and likely causes the delay.

This has nothing to do with the fix in bug 1383138, it's just a consequence of the async behavior. Probably before we were just blocking the main-thread (the UI) until ready to proceed. Now we are more responsive but the dialog is shown too early and we don't wait for the background operations to complete.
This is clearly more likely to happen for a large number of tabs.

Comment 3

[nivtwig]

(In reply to Marco Bonardo [::mak] from comment #2)
> > After updating to the latest Nightly 2017-07-30, a default bookmarks folder
> > with the name "[Folder Name]" was created, is not empty like with Nightly
> > 2017-07-26, but has only 76 tabs, most of the tabs are missing  (instead of
> > about 880 tabs ...)
> 
> Any errors reported to the browser console?

A lot of times, but not always, I get this error in all the following 3 cases: when I wait for the missing fields to appear and enter a folder name, when I wait for the missing fields to appear and leave the default folder name, and when I don't wait for the fields to appear and try to immediately press the "Add bookmarks" button.

(As I have written before, when I wait for the fields to appear, both when entering a folder name or leaving the default, everything is working properly, all the tabs are bookmarked, but this error still shows on the console)

TypeError: parentDocShell.getDocShellEnumerator is not a function[Learn More]  tab.js:62:23

And one time only I got this error (I don't remember in which case):
TypeError: this._batchBlockingDeferred is null[Learn More]  bookmarkProperties.js:384:9

> > After some tries, I found out later that if you wait long enough (about 30
> > seconds) after clicking "Bookmark all tabs", the missing 2 input fields
> > folder name and Folder (parent folder), suddenly appear (!) in the dialog
> > box, enlarging it, and giving me the option to type the folder name. When
> > this happens, all tabs seem to be bookmarked correctly, without missing
> > tabs. 
> 
> I think the problem is that we must collect information about all the tabs
> and create all the bookmarks before proceeding. 880 tabs is a lot to do that
> and likely causes the delay.
> 
> This has nothing to do with the fix in bug 1383138, it's just a consequence
> of the async behavior. Probably before we were just blocking the main-thread
> (the UI) until ready to proceed. Now we are more responsive but the dialog
> is shown too early and we don't wait for the background operations to
> complete.
> This is clearly more likely to happen for a large number of tabs.

*) Why can't the 2 fields appear immediately in the dialog box, before the tabs information collection, instead of after ?

Comment 4

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

(In reply to nivtwig from comment #3)
> *) Why can't the 2 fields appear immediately in the dialog box, before the
> tabs information collection, instead of after ?

They could, but that would not mean the dialog is "ready" yet, it is probably creating those 880 bookmarks in background.
We must profile where most of the work happens and figure out a strategy so that the dialog stays responsive and the operation is not interrupted in the middle.

Comment 5

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

56 is unaffected for release, because the feature (so far) is Nightly only. It is affected only until Nightly is 56.

Comment 6

[nivtwig]

(In reply to Marco Bonardo [::mak] (Away 4-20 August) from comment #2)
> 
> I think the problem is that we must collect information about all the tabs
> and create all the bookmarks before proceeding. 880 tabs is a lot to do that
> and likely causes the delay.
> 

I don't understand why do you have to collect information about the tabs when the dialog opens, instead of when the "Add bookmarks" button of the dialog is pressed ?

880 tabs does not seem to me "a lot to do", A modern computer should go over a list (or collection) of thousands of items (or tabs) and add to or create a list (or collection) of a thousand of items (or bookmarks) in memory, in probably less than a millisecond. 

If there is a slowness problem there, then it should be fixed and optimized, but I don't think there is a need to do this operation when the dialog opens, only when the "Add bookmarks" button is pressed.

If the in-memory operations are not the problem, but the writing to disk or database takes too much time, then it can be done in the background after the accept button is pressed in order to not hang the UI .

Comment 7

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

(In reply to nivtwig from comment #6)
> (In reply to Marco Bonardo [::mak] (Away 4-20 August) from comment #2)
> I don't understand why do you have to collect information about the tabs
> when the dialog opens, instead of when the "Add bookmarks" button of the
> dialog is pressed ?

Because it's an instant-apply dialog, all of the bookmark dialogs are and changing that is a large effort.

Comment 8

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

For how things are currently, I think we should try to use insertTree and disable the accept button until ready.

But I didn't have time to profile this, so it's hard to tell if the problem is collecting info, creating bookmarks, or both.

Comment 9

[Mark Banner (:standard8)]

I've been starting to dig into this a bit. Here's what I've found so far, based on experiments on my test profile with bookmarking ~600 tabs:

- Getting the uris & titles for all the tabs seems to be insignificant at the moment, probably less than 20ms.
- onDialogLoad is the main hit.
-- Using the old sync transactions, this is taking ~ 1 second.
-- Using the new async transactions, it takes about 3.5 - 4 seconds.
- The majority of time for onDialogLoad is _promiseNewItem, where we create the folder and then add all the bookmarks to it.

I'll try switching to insertTree as a first take, and see how that affects the timings.

Comment 10

[nivtwig]

(In reply to Mark Banner (:standard8) from comment #9)
> - onDialogLoad is the main hit.
> -- Using the old sync transactions, this is taking ~ 1 second.
> -- Using the new async transactions, it takes about 3.5 - 4 seconds.
> - The majority of time for onDialogLoad is _promiseNewItem, where we create
> the folder and then add all the bookmarks to it.

I am not sure we measured the same thing and that you are looking at the main problem or all the timings problems yet, since my timings were much worse : about 30 seconds for 800 tabs for the 2 input fields to show.

You write that you measured onDialogLoad , is it the same as measuring the time to the showing of the 2 input fields that I measured ?

I have no knowledge of the "Bookmark all tabs" dialog's code internals but it seems there are 2 main options:

1. if the browser "waits" for the onDialogLoad function to finish and the UI is not responsive in that time, and when it finishes it has all the information about all tabs, and immediately displays the 2 input fields, then this will be equal to what I measured, the time to show the 2 input fields. But then I wouldn't be able to click the "Add bookmarks" button before the 2 fields appear, while it was collecting all the tabs information before it was complete, and I wouldn't have a folder with missing tabs.

2. If on the other hand the onDialogLoad function only starts a collection of the tabs info and creation of bookmarks in the background, but when it finishes it doesn't wait for the tabs collection and bookmarks creation to complete, then what you measured (the time spent in onDialogLoad) is not equal to what I measured (the time to show the 2 input fields), but only a small part of it, and there are additional problems except the onDialogLoad timing.

Comment 11

[Mark Banner (:standard8)]

(In reply to nivtwig from comment #10)
> (In reply to Mark Banner (:standard8) from comment #9)
> > - onDialogLoad is the main hit.
> > -- Using the old sync transactions, this is taking ~ 1 second.
> > -- Using the new async transactions, it takes about 3.5 - 4 seconds.
> > - The majority of time for onDialogLoad is _promiseNewItem, where we create
> > the folder and then add all the bookmarks to it.
> 
> I am not sure we measured the same thing and that you are looking at the
> main problem or all the timings problems yet, since my timings were much
> worse : about 30 seconds for 800 tabs for the 2 input fields to show.

There's probably a little more overhead I haven't measured, but I'm covering the critical bit which appeared to make this worse - the transition from sync to async transactions.

Also, I'm on a recentish Mac with an SSD drive and 16Gb ram. I notice you're on Windows so your specs are likely to be largely different, and especially if you have an HDD drive then read/writes will be slower which could have a bigger impact here. Also it'll probably depend on extensions and other items in your profile.

> I have no knowledge of the "Bookmark all tabs" dialog's code internals but
> it seems there are 2 main options:

The problem I'm going to address directly here is to fix the immediate regression of async transactions. I've already prototyped it and I'm getting very similar timings to the old sync transactions.

As Marco said, we know there's other issues in the dialog which are more major to resolve, so we'll work on those in other bugs. I'll probably see if I can pick up bug 885246 soon after this which will help as well.

Comment 12

[nivtwig]

(In reply to Mark Banner (:standard8) from comment #11)
> Also, I'm on a recentish Mac with an SSD drive and 16Gb ram. I notice you're
> on Windows so your specs are likely to be largely different, and especially
> if you have an HDD drive then read/writes will be slower which could have a
> bigger impact here. Also it'll probably depend on extensions and other items
> in your profile.

I too have an SSD (Samsung 850 EVO), and 12GB of RAM on a 64-bit Windows 10, Intel G2020 CPU 2.9 GHZ, so I don't think my hardware is the cause for our differences, maybe the CPU is slower but I doubt it causes a 5-10X difference in timings.

> > I have no knowledge of the "Bookmark all tabs" dialog's code internals but
> > it seems there are 2 main options:
> 
> The problem I'm going to address directly here is to fix the immediate
> regression of async transactions. I've already prototyped it and I'm getting
> very similar timings to the old sync transactions.
> 
> As Marco said, we know there's other issues in the dialog which are more
> major to resolve, so we'll work on those in other bugs. I'll probably see if
> I can pick up bug 885246 soon after this which will help as well.

I don't understand from this what is the answer to my question, you didn't write clearly if the time you measured was the time in onLoadDialog, or the time for the showing of the 2 fields ?

Comment 13

[Mark Banner (:standard8)]

Attachment: Bug 1385733 - Improve the performance of async transactions when bookmarking all tabs.

Optimise adding a folder with child bookmarks for transactions by allowing PlacesTransactions.NewFolder to take children details and use insertTree rather than needing separate NewFolder and then multiple NewBookmark transactions.

Review commit: https://reviewboard.mozilla.org/r/164566/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/164566/

Comment 14

[Mark Banner (:standard8)]

The patch I've just attached is fixing the main performance regression which was from switching from sync to async transactions. I'll fix the accept button in bug 885246 next week.

(In reply to nivtwig from comment #12)
> I don't understand from this what is the answer to my question, you didn't
> write clearly if the time you measured was the time in onLoadDialog, or the
> time for the showing of the 2 fields ?

It was the time in onLoadDialog, however that should include the time for showing the elements as well, I can't see anything else happening outside it that would affect those.

Without a more precise regression point the transactions switch is our best guess at the issue here. We've other work to do that will improve the general performance of this and other parts of FF wrt bookmarks that will come over the next few weeks/months as well.

Comment 15

[Drew Willcoxon :adw]

Comment on attachment 8893491 [details]
Bug 1385733 - Improve the performance of async transactions when bookmarking all tabs.

https://reviewboard.mozilla.org/r/164566/#review170120

So the win here is replacing N PlacesTransactions.NewBookmark() calls with a single PlacesUtils.bookmarks.insertTree() call?  Am I reading it right?  Sounds good.

The only other comment I have is that switching from "uri" to "url" in various places is a strange choice IMO.  We pretty much always use "uri" for nsIURI objects and "url" (or "spec") for strings.

::: toolkit/components/places/PlacesTransactions.jsm:1137
(Diff revision 1)
> +      title,
> +      type: PlacesUtils.bookmarks.TYPE_FOLDER,
> +    };
> +
> +    if (children && children.length > 0) {
> +      info = {

Is info.parentGuid not needed in this case?  Because now you're not defining it in this branch, which doesn't seem right, but I don't know.  It looks like you replace parentGuid with guid?

::: toolkit/components/places/PlacesTransactions.jsm:1159
(Diff revision 1)
> +        // the child bookmarks, but we believe that isn't important at the moment.
> +        let bmInfo = await PlacesUtils.bookmarks.insertTree(info);
> +        // insertTree returns an array, but we only need to deal with the folder.
> +        folderGuid = bmInfo[0].guid;
> +      } else {
> -      info = await PlacesUtils.bookmarks.insert(info);
> +        info = await PlacesUtils.bookmarks.insert(info);

Not related to this bug, but it would be nice if insertTree() devolved to insert() if there are no children.  Or there were just one insert method that handled both cases.  (I say that not knowing much about these functions!)

Comment 16

[Mark Banner (:standard8)]

Comment on attachment 8893491 [details]
Bug 1385733 - Improve the performance of async transactions when bookmarking all tabs.

https://reviewboard.mozilla.org/r/164566/#review170120

> So the win here is replacing N PlacesTransactions.NewBookmark() calls with a single PlacesUtils.bookmarks.insertTree() call?  Am I reading it right?  Sounds good.

Yes that's right.

> The only other comment I have is that switching from "uri" to "url" in various places is a strange choice IMO.  We pretty much always use "uri" for nsIURI objects and "url" (or "spec") for strings.

Thanks for the info. I took another look and I've reverted most of it. The new API does use `url` rather than `uri` so I was trying to avoid a transition, but it seems that we want it anyway.

> Is info.parentGuid not needed in this case?  Because now you're not defining it in this branch, which doesn't seem right, but I don't know.  It looks like you replace parentGuid with guid?

Yeah that's right, insertTree takes a guid for what you're inserting the children into.

> Not related to this bug, but it would be nice if insertTree() devolved to insert() if there are no children.  Or there were just one insert method that handled both cases.  (I say that not knowing much about these functions!)

Your comment has made me realise that we *can* create just a single folder with insertTree, and hence we can simplify the transaction method. For some reason I'd got it into my head that it wasn't possible.

Hence, I'm updating the patch with a slightly revised, cleaner version that should work nicer overall.

Comment 17

[Mark Banner (:standard8)]

Comment on attachment 8893491 [details]
Bug 1385733 - Improve the performance of async transactions when bookmarking all tabs.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/164566/diff/1-2/

Comment 18

[Mark Banner (:standard8)]

Comment on attachment 8893491 [details]
Bug 1385733 - Improve the performance of async transactions when bookmarking all tabs.

Re-requesting review due to the additional changes/improvements.

Comment 19

[Mark Banner (:standard8)]

Comment on attachment 8893491 [details]
Bug 1385733 - Improve the performance of async transactions when bookmarking all tabs.

Grr, minor issue. insertTree does different handling with insertion indexes. I might have to revert to the previous option for now. I'll see what I can figure out.

Comment 20

[Mark Banner (:standard8)]

Comment on attachment 8893491 [details]
Bug 1385733 - Improve the performance of async transactions when bookmarking all tabs.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/164566/diff/2-3/

Comment 21

[Mark Banner (:standard8)]

Comment on attachment 8893491 [details]
Bug 1385733 - Improve the performance of async transactions when bookmarking all tabs.

Insertion point issue fixed, we should be good to go now.

Comment 22

[Drew Willcoxon :adw]

Comment on attachment 8893491 [details]
Bug 1385733 - Improve the performance of async transactions when bookmarking all tabs.

https://reviewboard.mozilla.org/r/164566/#review170950

Thanks Mark, looks good

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c072348c0360
Improve the performance of async transactions when bookmarking all tabs. r=adw

Comment 24

[Carsten Book [:Tomcat]]

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

Comment 25

[Jamie Charlton (:Jamie_)]

tested with both 32bit and 64bit
Verified fixed on current mozilla central builds
Version 	57.0a1
Build ID 	20170822031045
Mozilla/5.0 (X11; Linux x86_64; rv:57.0) Gecko/20100101 Firefox/57.0

Comment 26

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

nitwig, would you mind reporting if things are better for your use case as well?
While we fixed the functional issue, there could still be space for perf improvements and I'd like to understand how much it improved for you.

Comment 27

[nivtwig]

(In reply to Marco Bonardo [::mak] from comment #26)
> nitwig, would you mind reporting if things are better for your use case as
> well?
> While we fixed the functional issue, there could still be space for perf
> improvements and I'd like to understand how much it improved for you.

I see 3 bugs / problems :

1) With 880 tabs , there is still a delay of about 3 seconds before the input fields are shown and the "Add bookmarks" button is enabled. This should be immediate. 
This is much better than before the fix (it was about 30 seconds), but still not enough IMO.
Even if you don't want to fix this 3 seconds delay of data gathering for now, I see no reason why at least the input fields will not be shown immediately. There is no reason for them to "wait" for data gathering to be shown. 
Only the "Add bookmarks" button needs to be disabled until the data is ready, not the input fields, which can be enabled and shown immediately from the start and allow the user to immediately start typing while the data gathering happens asynchronously. 

2) There is a recent regression where the "Bookmark all tabs" dialog window is now transparent except for the input fields and buttons , I don't know if it was caused by the fix for this bug or not .
Tested with Nightly 57.0a1 (2017-08-22) (64-bit) 

3) With 77 tabs, there is a "blinking" of the Folder field contents : it changes from "Bookmarks Toolbar" and yellow icon at the left, to "Bookmarks menu" with a gray icon to the left.

With 880 tabs, this happens more slowly and takes 1-2 seconds. When the 2 input fields are shown and the "Add bookmarks" button changes from disabled to enabled after data gathering is probably finished, you see initially "Bookmarks toolbar" with a yellow icon at the beginning, and after 1-2 seconds, it changes to "Bookmarks Menu" with a gray icon. 
The field should be shown immediately with its final content without blinking/changing

Comment 28

[nivtwig]

(In reply to nivtwig from comment #27)
> 3) With 77 tabs, there is a "blinking" of the Folder field contents : it
> changes from "Bookmarks Toolbar" and yellow icon at the left, to "Bookmarks
> menu" with a gray icon to the left.
> 
> With 880 tabs, this happens more slowly and takes 1-2 seconds. When the 2
> input fields are shown and the "Add bookmarks" button changes from disabled
> to enabled after data gathering is probably finished, you see initially
> "Bookmarks toolbar" with a yellow icon at the beginning, and after 1-2
> seconds, it changes to "Bookmarks Menu" with a gray icon. 
> The field should be shown immediately with its final content without
> blinking/changing

I forgot to mention regarding issue 3, that I think it is a regression, I don't remember seeing it before, but I am not sure.

Comment 29

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

(In reply to nivtwig from comment #27)
> Only the "Add bookmarks" button needs to be disabled until the data is
> ready, not the input fields, which can be enabled and shown immediately from
> the start and allow the user to immediately start typing while the data
> gathering happens asynchronously.

Sounds like something we could have a follow-up bug on, but it may not be trivial to just show all the fields, because they are instant-apply, so there's some risk a change is applied to incomplete info. Surely it wouldn't hurt for the specific bookmark all tabs case, but all the code is shared with all the bookmark dialogs.
Note that we already have bugs to address obvious causes of slowdown when adding lots of bookmarks, and those are blocking bug 1382966 that is a priority. I suspect when those are fixed the 3 seconds may go down quite a bit, so I'd wait a little bit for those first.

> 2) There is a recent regression where the "Bookmark all tabs" dialog window
> is now transparent except for the input fields and buttons , I don't know if
> it was caused by the fix for this bug or not .
> Tested with Nightly 57.0a1 (2017-08-22) (64-bit) 

Bug 1392633, it should be fixed shortly and it's not due to this change.

> 3) With 77 tabs, there is a "blinking" of the Folder field contents : it
> changes from "Bookmarks Toolbar" and yellow icon at the left, to "Bookmarks
> menu" with a gray icon to the left.
> With 880 tabs, this happens more slowly and takes 1-2 seconds. When the 2
> input fields are shown and the "Add bookmarks" button changes from disabled
> to enabled after data gathering is probably finished, you see initially
> "Bookmarks toolbar" with a yellow icon at the beginning, and after 1-2
> seconds, it changes to "Bookmarks Menu" with a gray icon. 
> The field should be shown immediately with its final content without
> blinking/changing

Sounds like a UI polish bug, please file it apart.

Comment 30

[nivtwig]

(In reply to Marco Bonardo [::mak] from comment #29)
> Sounds like a UI polish bug, please file it apart.

Filed bug 1393129 for that