Closed Bug 1437250 Opened 2 years ago Closed 2 years ago

uncaught exception: Unexpected node when opening Bookmarks in Library

Categories

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

defect

Tracking

()

RESOLVED FIXED
Firefox 60
Tracking Status
firefox-esr52 --- unaffected
firefox58 --- unaffected
firefox59 --- unaffected
firefox60 --- fixed

People

(Reporter: magicp.jp, Assigned: standard8)

References

Details

(Keywords: regression)

Attachments

(2 files)

Attached image 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
Blocks: 1423896
Has Regression Range: --- → yes
Has STR: --- → yes
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?
Flags: needinfo?(standard8)
Keywords: regression
Priority: -- → P1
Assignee: nobody → standard8
Flags: needinfo?(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.
(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.
(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.
(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 on attachment 8952710 [details]
Bug 1437250 - Bookmark url queries shouldn't return folder shortcuts.

https://reviewboard.mozilla.org/r/221946/#review227828
Attachment #8952710 - Flags: review?(mak77) → review+
Pushed by mbanner@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f0668098ade0
Bookmark url queries shouldn't return folder shortcuts. r=mak
https://hg.mozilla.org/mozilla-central/rev/f0668098ade0
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 60
You need to log in before you can comment on or make changes to this bug.