1085291 - A bookmark node that is inserted by live-update code is missing bookmarkGuid value (was: Drag & drop bookmark from the toolbar to a folder not working)

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Colin Howard]

User Agent: Mozilla/5.0 (Windows NT 6.3; Win64; x64; rv:36.0) Gecko/20100101 Firefox/36.0
Build ID: 20141019030207

Steps to reproduce:

Bookmark a page
attempt to drag & drop to either the bookmarks bar or any other bookmarks folder on that bar 


Actual results:

Nothiong


Expected results:

Bookmark should be replicated in the new location

Comment 1

[Loic]

In FF36 with e10s enabled or disabled?

Comment 2

[Colin Howard]

e10s disabled

Comment 3

[Loic]

Did you test with a fresh profile?
https://support.mozilla.org/en-US/kb/profile-manager-create-and-remove-firefox-profiles

Same issue?

Comment 4

[Colin Howard]

I used my administrator account which has sync enabled. Same issue
I used another test user account with its own profile. Same issue AND system became unresponsive. NO alt>ctrl>del so forced shutdown by long press on power button.
that resulted in a v.slow system start so I then instigated a fresh clean restart.
Firefox utilising half a gig of memory - 12.5% of available RAM.
Tried to bookmark a recently viewed page from new tab - failed to even register it as a bookmark. Did get the pretty animation though :-)
Bookmarked a random page from a Google search which did create an entry in recent bookmarks but still won't let me drag and drop it.
Is that enough to make you panic?

Comment 5

[Loic]

[Tracking Requested - why for this release]:

I confirm it.

STR:
1) enable the bookmark toolbar
2) create a New folder in the bookmark toolbar
3) bookmark any page in the bookmark toolbar (drag and drom the favicon)
4) drag and drop the bookmark from the toolbar to the folder

Result: the folder stays empty and the bookmark is not moved from the toolbar to the folder.

NB: draging and dropping directly the webpage in the folder works and the page is bookmarked.

regression range:
good=2014-10-14
bad=2014-10-15
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=54217864bae9&tochange=62f0b771583c

Comment 6

[Loic]

Console loge:
aNode is undefined PlacesUtils.jsm:149

Comment 7

[Marco Bonardo [:mak]]

looks like a call to nodeIsFolder with an undefined node.

Comment 8

[Colin Howard]

Not sure if it helps but I have just accessed the saved bookmarks in "unsorted bookmarks" and all the customary actions are effective in that folder.
Obviously a temporary workaround but all is not lost.

Comment 9

[Asaf Romano (gone)]

Is this still broken after the checkin of bug 1083376? I cannot reproduce it using the following STR in current nightly build:
1. Fresh profile, bookmarks toolbar turned on.
2. Create a new folder on the bookmark toolbar using the context menu
3. D&D "Getting Started" to that folder.

Comment 10

[Colin Howard]

I'm not certain how this bug has morphed from my original report of Oct 20 which was "Drag & Drop from recent bookmarks folder to bookmarks toolbar not working". That defect is still apparent. Whether or not the current tag line "Drag & drop bookmark from the toolbar to a folder not working" has subsumed, complemented or simply replaced my original report eludes me.

Comment 11

[Loic]

(In reply to Mano (::mano, needinfo? for any questions; not reading general bugmail) from comment #9)
> Is this still broken after the checkin of bug 1083376? I cannot reproduce it
> using the following STR in current nightly build:
> 1. Fresh profile, bookmarks toolbar turned on.
> 2. Create a new folder on the bookmark toolbar using the context menu
> 3. D&D "Getting Started" to that folder.

No, bookmarks displayed in the bookmark toolbar cannot be moved in a folder of the bookmark toolbar or moved between them.

Comment 12

[Alice0775 White]

Steps To Reproduce: for example
1. Start Nightly with newly created profile and enable "Bookmarks Toolbar" (In order to simplify to reproduce the problem)
2. Create a new folder(named "New Folder") on the bookmark toolbar
3. D&D "Getting Started" to the "New Folder".
4. Open the "New Folder"
5. D&D "Getting Started" to "Bookmarks Toolbar"

This problem occurs, not only on the Bookmarks Toolbar, but also on the Bookmarks menu. 

Steps To Reproduce: for example
1. Start Nightly with newly created profile and enable menubar (In order to simplify to reproduce the problem)
2. Open "Bookmarks" menu in the menu bar
3. Open "Mozilla Firefox" folder
4. D&D "Help and Tutorials" to "Bookmarks" menupopup
5. D&D the "Help and Tutorials" to the "Mozilla Firefox" folder

Comment 13

[Asaf Romano (gone)]

Thanks!

So, this is only reproducible when the node is dropped "for the second time" in the same session (see the STR).

The reason for that, it seems, is that when a bookmark result node is added (nodeInserted etc) it lacks the bookmarkGuid property (meaning that a node with an itemId has no bookmarkGuid). The serialization code in PlacesUtils relies on the bookmarkGuid-existence check for deducing that a node is a bookmark. And so because bookmarkGuid is not set on the node, parent is not set on the unwrapped-node. Since canMoveUnwrappedNode (rightly) assumes that if the unwrapped-node it's dealing with has an itemId, it also has a parent, it passes the missing parent value to isContentsReadOnly.

This means that the fix for bug 1068671 simply uncovered a backend-bug.

Comment 14

[Asaf Romano (gone)]

And BookmarkIdToResultNode - which, indeed, doesn't select moz_bookmarks.guid along with few other moz_bookmark properties - is in nsNavHistory.cpp... There it calls RowToReuslt which doesn't support those properties (they're defined in nsNavBookmarks.cpp).

Comment 15

[Asaf Romano (gone)]

Attachment: minimal patch

I didn't bother testing the onItemMove path. It's basically onItemAdded + a bug in the date-added/lastModified values (they're set to dummy values).

Comment 16

[Marco Bonardo [:mak]]

Comment on attachment 8527317 [details] [diff] [review]
minimal patch

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

just one possible mistake, and some cleanups required...

::: toolkit/components/places/nsNavBookmarks.h
@@ +212,5 @@
>  
> +  static const int32_t kGetChildrenIndex_Position;
> +  static const int32_t kGetChildrenIndex_Type;
> +  static const int32_t kGetChildrenIndex_PlaceID;
> +  static const int32_t kGetChildrenIndex_Guid;

since you are moving them could you please reorder these based on their value (thus: guid, position, type, placeid)

::: toolkit/components/places/nsNavHistory.cpp
@@ +3932,5 @@
>        // RESULTS_AS_TAG_QUERY has date columns
>        resultNode->mDateAdded = aRow->AsInt64(kGetInfoIndex_ItemDateAdded);
>        resultNode->mLastModified = aRow->AsInt64(kGetInfoIndex_ItemLastModified);
>      }
>      else if (resultNode->IsFolder()) {

won't the itemId != -1 check you added above shadow this IsFolder check?
you should probably convert this else if into an if.

I''m not sure what this breaks though, the below comment is not very clear, you added it here:
http://hg.mozilla.org/mozilla-central/diff/6e48e6575417/toolkit/components/places/src/nsNavHistory.cpp

I guess it may break includeItems in the library left pane?

::: toolkit/components/places/nsNavHistory.h
@@ +221,5 @@
> +  // Additional moz_bookmarks fields.
> +  static const int32_t kGetChildrenIndex_Position;
> +  static const int32_t kGetChildrenIndex_Type;
> +  static const int32_t kGetChildrenIndex_PlaceID;
> +  static const int32_t kGetChildrenIndex_Guid;

do we really need this duplication, can't we refer to them as nsNavBookmarks::*?
Cause otherwise I'd vote to move all these const to an Helpers.h class

::: toolkit/components/places/tests/unit/test_1085291.js
@@ +1,3 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */

it should be PD, but since now the default for tests is PD, you can now completely omit the license header in tests and they will default to PD.

@@ +5,5 @@
> +add_task(function* () {
> +  // test that nodes inserted by incremental update for bookmarks of all types
> +  // have the extra bookmark properties (bookmarkGuid, dateAdded, lastModified).
> +
> +  // getFoldrContents opens the root node.

typo: Foldr

@@ +35,5 @@
> +                      , title: "Test Folder" });
> +
> +  // separator
> +  yield insertAndTest({ parentGuid: root.bookmarkGuid
> +                      , type: PlacesUtils.bookmarks.TYPE_SEPARATOR });

please root.containerOpen = false; at the end.

I know it's not mandatory, but it's a good habit to show to people looking at our tests.

Comment 17

[Asaf Romano (gone)]

https://hg.mozilla.org/integration/fx-team/rev/6db486ed2de1

Comment 18

[Asaf Romano (gone)]

> ::: toolkit/components/places/nsNavHistory.cpp
> @@ +3932,5 @@
> >        // RESULTS_AS_TAG_QUERY has date columns
> >        resultNode->mDateAdded = aRow->AsInt64(kGetInfoIndex_ItemDateAdded);
> >        resultNode->mLastModified = aRow->AsInt64(kGetInfoIndex_ItemLastModified);
> >      }
> >      else if (resultNode->IsFolder()) {
> 
> won't the itemId != -1 check you added above shadow this IsFolder check?
> you should probably convert this else if into an if.
> 
> I''m not sure what this breaks though, the below comment is not very clear,
> you added it here:
> http://hg.mozilla.org/mozilla-central/diff/6e48e6575417/toolkit/components/
> places/src/nsNavHistory.cpp
> 
> I guess it may break includeItems in the library left pane?
> 

It did shadow, as you suggested, but changing it to an unconditioned else block caught too much, and broke test_containersQueries_sorting.js.

So I chained it under the if block instead. We might be setting dateAdded and lastModified twice in some cases, but that's not really a problem.

> ::: toolkit/components/places/nsNavHistory.h
> @@ +221,5 @@
> > +  // Additional moz_bookmarks fields.
> > +  static const int32_t kGetChildrenIndex_Position;
> > +  static const int32_t kGetChildrenIndex_Type;
> > +  static const int32_t kGetChildrenIndex_PlaceID;
> > +  static const int32_t kGetChildrenIndex_Guid;
> 
> do we really need this duplication, can't we refer to them as
> nsNavBookmarks::*?
> Cause otherwise I'd vote to move all these const to an Helpers.h class
> 

Fixed. They were already referenced as nsNavBookmarks::*. It was a left over from a previous version.

> ::: toolkit/components/places/tests/unit/test_1085291.js
> @@ +1,3 @@
> > +/* This Source Code Form is subject to the terms of the Mozilla Public
> > + * License, v. 2.0. If a copy of the MPL was not distributed with this
> > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> 
> it should be PD, but since now the default for tests is PD, you can now
> completely omit the license header in tests and they will default to PD.
> 

removed it.

Comment 19

[Carsten Book [:Tomcat]]

sorry had to back this out for causing bustages like https://treeherder.mozilla.org/ui/logviewer.html#?job_id=1284337&repo=fx-team

Comment 20

[Asaf Romano (gone)]

The test file was missing.

https://hg.mozilla.org/integration/fx-team/rev/68ddf70fa7c8

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/68ddf70fa7c8

Comment 24

[Andrei Vaida [:avaida]]

Reproduced the initial bug with an older nightly from 2014-10-20.

This is now verified fixed on Nightly 37.0a1 (2014-12-01) and Aurora 36.0a2 (2014-12-01), using Windows 7 64-bit, Ubuntu 12.04 LTS 32-bit and Mac OS X 10.9.5.