1437250 - uncaught exception: Unexpected node when opening Bookmarks in Library

Status: RESOLVED FIXED

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

Description

[magicp]

Attachment: recently-bookmarked.png

Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:60.0) Gecko/20100101 Firefox/60.0 ID:20180209220324

Steps to reproduce:
1. Launch Nightly
2. Open Library button > Bookmarks


Actual results:
The following errors are logged in the browser console.

uncaught exception: Unexpected node  browserPlacesViews.js:91:7
uncaught exception: Unexpected node  PanelMultiView.jsm:128:5


Reproducible situation:
There is no recently bookmarked item, but it is different with "(empty)". So this cannot reproduce with a newer profile. Please find the attached image.


Expected results:
No errors.


Regression range:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=12de5643ae0ae6ad6a97797f2a07fe856f9b66e1&tochange=c388570c330fd7745f12eca3258cc3ae4b3d5bb2

Comment 1

[Marco Bonardo [:mak]]

Mark, do you have any time to take a look at this?

It sounds like a failure in _createDOMNodeForPlacesNode. Could be the nodes we autogenerate are somehow missing the type?

Comment 2

[Mark Banner (:standard8)]

Initial findings:

- As comment 0 says, this doesn't happen on newer profiles.
- The query being run is: place:queryType=1&sort=12&maxResults=42&excludeQueries=1
- On one of my dev profiles, this hits:

https://searchfox.org/mozilla-central/rev/b9f1a4ecba48b2d8c686669e32d109c40e927b48/browser/components/places/content/browserPlacesViews.js#2212

The type is a folder shortcut (9) which is not one of the root folders, but points to the toolbar folder, created during my testing when writing the initial patch.

- On another dev profile, the menu worked just fine. However, I can reproduce then this by dragging & dropping e.g. "Other Bookmarks" folder to a different sub-folder.


So I think there's two issues here:

1) How to handle folder shortcuts in the places panel (we probably shouldn't be displaying them).
2) The drag and drop is now creating folder shortcuts. I thought I'd fixed that before landing the patch, but maybe not.

Comment 3

[Marco Bonardo [:mak]]

(In reply to Mark Banner (:standard8) from comment #2)
> 1) How to handle folder shortcuts in the places panel (we probably shouldn't
> be displaying them).

What is the Places Panel here? A folder shortcut is pretty much a folder, its contents are writable, why should we hide it?

> 2) The drag and drop is now creating folder shortcuts. I thought I'd fixed
> that before landing the patch, but maybe not.

So currently copying a folder shortcut creates a folder shortcut. That could also be ok, on the other side copying a folder creates a copy of its contents, and shortcuts are unrecognizeable from folders in the UI, so the whole thing is a bit confusing in general... For now we should probably go back to the status-quo that was making a copy of the contents, as you suggest.

Comment 4

[Mark Banner (:standard8)]

(In reply to Marco Bonardo [::mak] from comment #3)
> (In reply to Mark Banner (:standard8) from comment #2)
> > 1) How to handle folder shortcuts in the places panel (we probably shouldn't
> > be displaying them).
> 
> What is the Places Panel here? A folder shortcut is pretty much a folder,
> its contents are writable, why should we hide it?

Sorry, I was referring to the "PlacesPanelview" code which seems to drive the "Recently Bookmarked" list from the "View history, saved bookmarks & more" button on the Toolbar (and then click Bookmarks).

I'd have thought we wouldn't want folder shortcuts/folders to be displayed there.

Comment 5

[Marco Bonardo [:mak]]

(In reply to Mark Banner (:standard8) from comment #4)
> I'd have thought we wouldn't want folder shortcuts/folders to be displayed
> there.

Yes, that menu should NOT show folders, it's a plain list of recent bookmarks.

OT: There is actually another bug in that menu, that is bookmarks dragged from it are being copied instead of moved, that is a bug in general of any query returning bookmarks. The original reason for the choice of queries copying contents is that when you search in the library you don't know where the bookmark comes from, thus it's hard to "organize things" when you don't know the origin. On the other side, this causes the user to create copies and copies of the same bookmarks around. FWIW, that is bug 1410896.

Comment 6

[Mark Banner (:standard8)]

Attachment: Bug 1437250 - Bookmark url queries shouldn't return folder shortcuts.

URL queries don't need to return queries not shortcuts, as that isn't what the invoker is expecting.

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

Comment 7

[Marco Bonardo [:mak]]

Comment on attachment 8952710 [details]
Bug 1437250 - Bookmark url queries shouldn't return folder shortcuts.

https://reviewboard.mozilla.org/r/221946/#review227828

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f0668098ade0
Bookmark url queries shouldn't return folder shortcuts. r=mak

Comment 9

[Natalia Csoregi [:nataliaCs]]

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