Closed Bug 1085291 Opened 10 years ago Closed 9 years ago

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)

Categories

(Firefox :: Bookmarks & History, defect)

36 Branch
x86
Windows 8.1
defect
Not set
major
Points:
8

Tracking

()

VERIFIED FIXED
Firefox 36
Iteration:
37.1
Tracking Status
firefox35 --- unaffected
firefox36 + verified
firefox37 --- verified

People

(Reporter: that.man.colin, Assigned: asaf)

References

Details

(Keywords: regression)

Attachments

(1 file)

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
In FF36 with e10s enabled or disabled?
Component: Untriaged → Drag and Drop
Flags: needinfo?(that.man.colin)
Product: Firefox → Core
e10s disabled
Flags: needinfo?(that.man.colin)
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?
[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
Blocks: 1068671
Status: UNCONFIRMED → NEW
Component: Drag and Drop → Bookmarks & History
Ever confirmed: true
Flags: needinfo?(mano)
Keywords: regression
Product: Core → Firefox
Summary: Drag & drop from recent bookmarks not working → Drag & drop bookmak from the toolbar to a folder not working
Summary: Drag & drop bookmak from the toolbar to a folder not working → Drag & drop bookmark from the toolbar to a folder not working
Console loge:
aNode is undefined PlacesUtils.jsm:149
looks like a call to nodeIsFolder with an undefined node.
Flags: qe-verify+
Flags: in-testsuite?
Flags: firefox-backlog+
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.
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.
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.
(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.
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
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.
Assignee: nobody → mano
Flags: needinfo?(mano)
Summary: Drag & drop bookmark from the toolbar to a folder not working → 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)
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).
Points: --- → 8
Status: NEW → ASSIGNED
Iteration: --- → 36.3
Attached patch minimal patchSplinter Review
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).
Attachment #8527317 - Flags: review?(mak77)
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.
Attachment #8527317 - Flags: review?(mak77) → review+
> ::: 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.
Iteration: 36.3 → 37.1
https://hg.mozilla.org/mozilla-central/rev/68ddf70fa7c8
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Flags: in-testsuite? → in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 36
Depends on: 1105208
QA Contact: andrei.vaida
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.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.