1345349 - Bookmark keywords cannot be used or updated if a keyword points to an invalid url

Status: RESOLVED FIXED

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

Description

[zebediah49]

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:52.0) Gecko/20100101 Firefox/52.0
Build ID: 20170303013352

Steps to reproduce:

Updated to Firefox 52.0 (64-bit) (Ubuntu 12.04 edition)


Actual results:

I attempted to use a keyword-triggered quicksearch, and got a regular search for the keyword.  In other words, typing "g firefox" to search for firefox with the keyword 'g' forwarding to google, I instead was brought to the default search page, searching for the entire string "g firefox".

I went into bookmarks to investigate, and found that all of my quick-searches had no keyword any more.

Furthermore, attempting to enter a value for the keyword did not help.  Instead, every single bookmark shows the same value for the keyword field (as if the text box isn't even connected to anything).

Attempting to restore my bookmarks via the 'import and backup' dialog to a backup from today simply returns "Unable to process backup file".


Expected results:

1. Quick searches should quick-search.
2. Setting a keyword for a quick-search should actually set the keyword.
2a. Setting a keyword for one bookmark should only affect that bookmark's keyword
3. Restoring the bookmark settings should restore them.

Comment 1

[Marco Bonardo [:mak]]

Try cleaning up your db with this add-on, then try again adding your keyword
https://addons.mozilla.org/en-US/firefox/addon/places-maintenance/

Comment 2

[zebediah49]

(In reply to Marco Bonardo [::mak] from comment #1)
> Try cleaning up your db with this add-on, then try again adding your keyword
> https://addons.mozilla.org/en-US/firefox/addon/places-maintenance/

No problems found; as far as I can tell it didn't change much (final DB size == initial size).  Keyword problem persists.

Comment 3

[zebediah49]

I have pinpointed and resolved the issue, at least for myself.  

I noticed that suggestions of history also wasn't working, so I tried running firefox with a new profile, which did not have the issue.  In the process, running as my user, I got a significant number of errors to console, all of which said

A coding exception was thrown and uncaught in a Task.

    Full message: TypeError: http://document.body.innerhtml=document.body.innerhtml.replace("img/mask_v2%22,%20%22photo/%22+document.URL.split(%27/%27)%5B4%5D+%22_700b%22); is not a valid URL.
    Full stack: @resource://gre/modules/PlacesUtils.jsm:2333:28
    TaskImpl_run@resource://gre/modules/Task.jsm:319:42
    Handler.prototype.process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:932:23
    this.PromiseWalker.walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:813:7
    this.PromiseWalker.scheduleWalkerLoop/<@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:747:11

Following this, PlacesUtils.jsm:2333 calls the SQL query `SELECT keyword, url, post_data FROM moz_keywords k JOIN moz_places h ON h.id = k.place_id;`, which I replicated myself, and found that there was a keyword (it must have been there from ages ago, because I don't remember it) with the "url" ` http://document.body.innerhtml=document.body.innerhtml.replace("img/mask_v2%22,%20%22photo/%22+document.URL.split(%27/%27)%5B4%5D+%22_700b%22);`.  Exiting firefox, deleting the keyword for it directly from the SQL (`DELETE FROM moz_keywords WHERE keyword='<keyword>'`), and restarting firefox returned it to usable condition.

It would appear that a change introduced in 52.0 causes keyword-triggered bookmarlets to destroy the entire functionality of the keyword system.

Comment 4

[Marco Bonardo [:mak]]

(In reply to zebediah49 from comment #3)
>     Full message: TypeError:
> http://document.body.innerhtml=document.body.innerhtml.replace("img/
> mask_v2%22,%20%22photo/%22+document.URL.split(%27/%27)%5B4%5D+%22_700b%22);
> is not a valid URL.

> It would appear that a change introduced in 52.0 causes keyword-triggered
> bookmarlets to destroy the entire functionality of the keyword system.

The problem is different, that url is invalid and any code trying to resolve it would throw. Other features could have been broken due to it.
Now, what I can't explain is that all of our APIs pass any url through sanitization, so it's impossible for Firefox itself to add that url into the database. The only way would be to directly insert it through a raw SQL query or editing in a Database Manager of some sort.

Maybe it was added by an add-on?
Have you ever edited the database manually through SQL in the past?

It could be worth, on our side, to add a task to db maintenance to test all the urls and remove invalid ones.

Comment 5

[Marco Bonardo [:mak]]

and we could also add a try/catch around keywords fetching to eventually paper over an invalid url.

Comment 6

[Marco Bonardo [:mak]]

Attachment: Bug 1345349 - Places keywords break if one of the keywords points to an invalid url

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

Comment 7

[Mark Banner (:standard8)]

Comment on attachment 8867263 [details]
Bug 1345349 - Places keywords break if one of the keywords points to an invalid url

https://reviewboard.mozilla.org/r/138802/#review142464

Looks good. r=Standard8

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4500a7b7bbd5
Places keywords break if one of the keywords points to an invalid url r=standard8

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/4500a7b7bbd5