Closed Bug 1401401 Opened 2 years ago Closed 6 months ago

Bookmarks with an invalid url cannot be removed

Categories

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

55 Branch
defect
Points:
3

Tracking

()

VERIFIED FIXED
Firefox 69
Iteration:
69.2 - May 27 - Jun 9
Tracking Status
firefox69 --- verified

People

(Reporter: original.roju, Assigned: mak)

References

(Blocks 3 open bugs)

Details

(Whiteboard: [fxsearch])

Attachments

(2 files, 1 obsolete file)

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.12; rv:55.0) Gecko/20100101 Firefox/55.0
Build ID: 20170824053838

Steps to reproduce:

Platform: macOS 10.12.6
Firefox: 55.0.3 (64-bit)

This profile is quite old. I was doing some cleanup and noticed a bookmark in my toolbar that cannot be edited or deleted.

Scenario A
1. Right click the bookmark and choose properties

Scenario A1
1. Right click the bookmark and choose delete

Scenario B
1. Open the "Show all bookmarks dialog"
2. Click this bookmark to view its details
3. Right click and hit delete

Scenario C
1. Using the WebExtensions bookmarks API, enumerate the bookmarks



Actual results:

Scenario A
1. Dialog appears, but is missing all the textboxes and buttons

Scenario A1
1. Nothing happens

Scenario B
1. The details panel does not show the details
2. Delete does nothing

Scenario C
1. Details on this bookmark should be displayed


Expected results:

A: Properties should show

A1: Bookmark should be deleted

B: Info should display. Delete should work.

C: The bookmark does not show up through the bookmarks API

Examining places.db, the entry looks like (with some details omitted with [snip]:

sqlite> select * from moz_bookmarks where parent=3;
[snip]
175|1|82|3|9| |||[snip]|[snip]|[snip]|2|0

sqlite> select * from moz_places where id = 82
   ...> ;
82|http:///||.|0|0|0||82||[snip]|1|[snip]
"http:///" is indeed an invalid url and that's likely why this happens.

The fact is, the API never inserts invalid urls because all the urls go through nsIURI or URL objects, so they are always validated.

Do you have any idea of how this invalid url may have finished into the database? Did you have any add-ons that could have run raw SQL queries on the database?

We surely need some way to allow the user to remove a bookmark pointing to an invalid url. this is likely a dupe of some old bugs, but still confirming to have it on track.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(original.roju)
Priority: -- → P2
Summary: Bookmark in bookmarks toolbar that cannot be edited or deleted → Bookmark in bookmarks toolbar that cannot be edited or deleted because it has an invalid url
Whiteboard: [fxsearch]
(In reply to Marco Bonardo [::mak] from comment #1)
> Do you have any idea of how this invalid url may have finished into the
> database? Did you have any add-ons that could have run raw SQL queries on
> the database?

I have no recollection of specifically trying to add it or why it might be there.

Recently, I have been running the containers and activity stream experiments. Further back, I used to have some addons that could be factors: one that would vacuum the database, Prettier Bookmarks Toolbar, Edit Bookmark Plus.

Checking the database, the bookmark was added Nov 23, 2016, just under a week after Firefox 50 release. So it could have been connected to that, whether an addon interaction with the update or something else.
Flags: needinfo?(original.roju)
Blocks: 1410877
Duplicate of this bug: 1410928
We should probably at least allow to remove the bookmark, that could mean skipping PlacesTransactions for this specific case.
As an alternative, we could maybe annotate somewhere invalid urls found by our APIs and act on them through maintenance.
Let's check again once the old transactions code is gone, there may be a simple point where to hook such a check.
From bug 1351282:

We should probably add some validation cleanup to the maintenance task.
It's strange that a previous version allowed to insert an invalid url, since all the official APIs always checked url validity. Maybe it was an add-on you had on previous versions that became incompatible now? Or maybe we just changed validation rules for nsIURI, lately something relevant happened there.

I agree that we should also make somehow possible to remove it.
Duplicate of this bug: 1351282
See Also: → 1470043
Duplicate of this bug: 1553377
Duplicate of this bug: 1554202

(In reply to Mark Banner (:standard8) (afk until 29th May) from comment #5)

From bug 1351282:

Or maybe we just
changed validation rules for nsIURI, lately something relevant happened
there.
I agree that we should also make somehow possible to remove it.

We changed the validation rules for URLs. I think we should either automatically remove the bookmarks with URIs that fail to parse, or not try to parse it at all when the bookmark is being removed, otherwise we'll bail out early.

The only problem left is how to do it, checking 150k uris is not cheap, we'll have to do that in chunks, probably by storing the latest verified id in a pref (we should be far from the maximum 64bit integer, so it's unlikely ids are being reused yet). It'd work but it's a bit fragile and expensive, especially when there are no broken urls.
The approach I was thinking about was more a passive one, when we hit a case where we fail the url check in a bookmark API, we add that url to a list (table? flag?), idle expiration may take care of removing those urls. Though we must pay particular attention to how Sync will handle those removals, it should not check url validity.

Duplicate of this bug: 1557347
Blocks: 1557445

I'm moving the more complex task of identifying and handling invalid urls to bug 1557445, while here we'll just handle the reported problem, that a bookmark with an invalid url cannot be removed.

Assignee: nobody → mak77
Status: NEW → ASSIGNED
Summary: Bookmark in bookmarks toolbar that cannot be edited or deleted because it has an invalid url → Bookmarks with an invalid url cannot be removed
Iteration: --- → 69.2 - May 27 - Jun 9
Points: --- → 3

This ignores invalid urls when PlacesUtils.bookmarks.remove is invoked, so that
these bogus entries can be removed. The resulting bookmark has a null uri.

bug 1557445 will look into a longer term strategy to handle invalid urls
in the database.

Pushed by mak77@bonardo.net:
https://hg.mozilla.org/integration/autoland/rev/72f2324b5358
Bookmarks with an invalid URL cannot be removed. r=Standard8,lina
Status: ASSIGNED → RESOLVED
Closed: 6 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 69

Comment on attachment 9070351 [details]
Bug 1401401 - Bookmarks with an invalid URL cannot be removed. r=Standard8,lina

Beta/Release Uplift Approval Request

  • User impact if declined: Bookmarks for invalid URLs cannot be removed. Those URLs can't be added by the user, but we may have changed validation rules so that they were added in the past and NOW they are invalid.
    See also bug 1548306
  • Is this code covered by automated tests?: Yes
  • Has the fix been verified in Nightly?: No
  • Needs manual test from QE?: Yes
  • If yes, steps to reproduce: It's not possible to add an invalid URL, so it should either be injected in the db manually, or use a build before bug 1548306 was fixed, and create a bookmark to a url that bug 1548306 made invalid.
    So, while it would be nice to have QA confirmation, it may not be trivial.
  • List of other uplifts needed: None
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): pretty much handling an exception.
  • String changes made/needed:
Attachment #9070351 - Flags: approval-mozilla-beta?
Flags: qe-verify+

Comment on attachment 9070351 [details]
Bug 1401401 - Bookmarks with an invalid URL cannot be removed. r=Standard8,lina

ESR Uplift Approval Request

  • If this is not a sec:{high,crit} bug, please state case for ESR consideration: Bug 1548306 and previous URL validation rule changes.
  • User impact if declined: Cannot remove a bookmark to an invalid url. See beta uplift request.
  • Fix Landed on Version:
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): See beta uplift request.
  • String or UUID changes made by this patch:
Attachment #9070351 - Flags: approval-mozilla-esr60?
QA Whiteboard: [qa-triaged]

Still reproducible on the latest Nightly, check recording: https://streamable.com/vpc88

Steps to reproduce:

  • Create an invalid bookmark, such as "https:///" via Firefox 50
  • Open the same profile with the latest Nightly
  • Try to delete or check properties for that bookmark

Marco, should I re-open this or log a new issue?

Flags: needinfo?(mak77)

Comment on attachment 9070351 [details]
Bug 1401401 - Bookmarks with an invalid URL cannot be removed. r=Standard8,lina

thanks Marco. approved for 68.0b10

Attachment #9070351 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Comment on attachment 9070351 [details]
Bug 1401401 - Bookmarks with an invalid URL cannot be removed. r=Standard8,lina

back to ? so Timea's comment can be addressed.

Attachment #9070351 - Flags: approval-mozilla-beta+ → approval-mozilla-beta?

First, note that the only thing that this would like to fix is the Remove action, we can't fix the Properties dialog nor any other UI pieces, because it would be too much complicated.

I'm sorry, better to reopen this. The patch is harmless, but also not useful.
My suspect is that, even if now the API works, PlacesTransactions stays in the middle and bailouts even before invoking the API. So we need a followup.

Do you see anything in the browser console? I suspect you may see a "Failed to get info for the specified item" error.

Status: RESOLVED → REOPENED
Flags: needinfo?(mak77) → needinfo?(timea.babos)
Resolution: FIXED → ---
Attachment #9070351 - Flags: approval-mozilla-esr60?
Attachment #9070351 - Flags: approval-mozilla-beta?

Yes, if I choose to Delete the bookmark I get:

  • "Error: "Failed to get info for the specified item (guid: llexfB107P1T): Error: Failed to fetch the data for the root item [Exception... "Component returned failure code: 0x804b000a (NS_ERROR_MALFORMED_URI) [nsIIOService.newURI]" nsresult: "0x804b000a (NS_ERROR_MALFORMED_URI)" location: "JS frame :: resource://gre/modules/NetUtil.jsm :: NetUtil_newURI :: line 184" data: no]"

If I choose Properties for the bookmark:

  • "Failed to initialize dialog: [Exception... "Component returned failure code: 0x804b000a (NS_ERROR_MALFORMED_URI) [nsIIOService.newURI]" nsresult: "0x804b000a (NS_ERROR_MALFORMED_URI)" location: "JS frame :: chrome://browser/content/places/editBookmark.js :: _setPaneInfo :: line 43" data: no]"
Flags: needinfo?(timea.babos)
Attachment #8910035 - Attachment is obsolete: true
Pushed by mak77@bonardo.net:
https://hg.mozilla.org/integration/autoland/rev/13b6a164d370
Bookmarks with an invalid url cannot be removed (part 2). r=Standard8
Status: REOPENED → RESOLVED
Closed: 6 months ago6 months ago
Resolution: --- → FIXED

Tried to verify the fix but the saved invalid bookmark does not show up in the bookmarks toolbar anymore after opening the latest Firefox version with the same profile. So basically, I can't try out the delete option.

Is this part of the intended fix, Marco?

Flags: needinfo?(mak77)

No, we don't remove bookmarks without user input. There must be something else ongoing. Which steps are you following, maybe I can try the same.

Flags: needinfo?(mak77) → needinfo?(tbabos)

Following the same steps as in Comment 19.
You also mentioned about injecting bookmarks in the db manually. I don't know how to do that, but if you could guide me it would be great.

Flags: needinfo?(tbabos)
Flags: needinfo?(mak77)
Blocks: 1568465

I created a profile with Firefox 66, created a bookmark to http://%.mozilla.org/ on the toolbar.
I opened the same profile with Nightly, tried to remove the bookmark, a bunch of errors are shown in the console, the bookmark persists in the UI. But restarting Firefox doesn't show the bookmark anymore, it has been removed.
So, the bookmark can be removed, unfortunately the UI doesn't handle notifications properly, it's likely because it's notifying a bookmark without a url that is quite unexpected. I filed bug 1568465 for the UI live-update problem.

Flags: needinfo?(mak77)

Sorry for leaving this hanging like that.
Tried again following your steps in comment 30 and had the same results on Beta 69 and Nightly 71. The bookmark gets removed but it is shown in the UI only after restarting the browser.

Will close this bug as verified - fixed as the UI part is handled in Bug 1568465.

Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.