Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Handle invalid URLs in bookmarks_get_all_with_url #1198

Closed
linabutler opened this issue May 25, 2019 · 1 comment
Closed

Handle invalid URLs in bookmarks_get_all_with_url #1198

linabutler opened this issue May 25, 2019 · 1 comment

Comments

@linabutler
Copy link
Contributor

linabutler commented May 25, 2019 •
edited
Loading

See mozilla/firefox-preview-bughunting-campaign#130. Typing 192.168.1.1:1234 into the address bar logs this:

05-24 17:16:16.635 30159 30213 E places::ffi: Unexpected sync error: Error(
05-24 17:16:16.635 30159 30213 E places::ffi:
05-24 17:16:16.635 30159 30213 E places::ffi: Error synchronizing: URL parse error: relative URL without a base)

And here's the crash. (I think the "error synchronizing" message is because parse_url returns a sync15::Result. The stack in Sentry points to getBookmarksWithUrl).

Could we give bookmarks_get_all_with_url the same treatment as places_get_visited in #554?

Should we also consider swallowing URL parse errors for places_delete_{place, visit}, too? We shouldn't record invalid URLs, but if one somehow sneaks into the database, it would be unfortunate if you couldn't delete it. Desktop is finding this out the hard way for bookmarks with invalid URLs.
@thomcc
Copy link
Contributor

thomcc commented May 25, 2019

We should probably do more work to clean up URLs that are recorded as history too.

thomcc pushed a commit that referenced this issue May 28, 2019
Allow invalid urls in bookmarks_get_all_with_url. Fixes #1198
d388271
@thomcc thomcc mentioned this issue May 28, 2019
Closed
@thomcc thomcc closed this as completed in 7a4a2c1 May 28, 2019
thomcc pushed a commit that referenced this issue May 28, 2019
Merge pull request #1205 from mozilla/get-all-with-url-invalid
e957c3b 
Allow invalid urls in bookmarks_get_all_with_url. Fixes #1198
@linabutler linabutler mentioned this issue Jun 4, 2019
Closed
badboy added a commit to badboy/application-services that referenced this issue Sep 18, 2020
@badboy
Upgrade Glean to v32.4.0
319da93 
Glean changelog:

* General
  * Allow using quantity metric type outside of Gecko ([mozilla#1198](mozilla/glean#1198))
  * Update `glean_parser` to 1.28.5
    * The `SUPERFLUOUS_NO_LINT` warning has been removed from the glinter. It likely did more harm than good, and makes it hard to make metrics.yaml files that pass across different versions of `glean_parser`.
    * Expired metrics will now produce a linter warning, `EXPIRED_METRIC`.
    * Expiry dates that are more than 730 days (~2 years) in the future will produce a linter warning, `EXPIRATION_DATE_TOO_FAR`.
    * Allow using the Quantity metric type outside of Gecko.
    * New parser configs `custom_is_expired` and `custom_validate_expires` added. These are both functions that take the expires value of the metric and return a bool. (See `Metric.is_expired` and `Metric.validate_expires`). These will allow FOG to provide custom validation for its version-based `expires` values.
  * Add a limit of 250 pending ping files. ([mozilla#1217](mozilla/glean#1217)).

Note: This also gets rid of the 2 workarounds (removed code) in
AppService thanks to upstream changes.
badboy added a commit to badboy/application-services that referenced this issue Sep 21, 2020
@badboy
Upgrade Glean to v32.4.0
5b3f4cf 
Glean changelog:

* General
  * Allow using quantity metric type outside of Gecko ([mozilla#1198](mozilla/glean#1198))
  * Update `glean_parser` to 1.28.5
    * The `SUPERFLUOUS_NO_LINT` warning has been removed from the glinter. It likely did more harm than good, and makes it hard to make metrics.yaml files that pass across different versions of `glean_parser`.
    * Expired metrics will now produce a linter warning, `EXPIRED_METRIC`.
    * Expiry dates that are more than 730 days (~2 years) in the future will produce a linter warning, `EXPIRATION_DATE_TOO_FAR`.
    * Allow using the Quantity metric type outside of Gecko.
    * New parser configs `custom_is_expired` and `custom_validate_expires` added. These are both functions that take the expires value of the metric and return a bool. (See `Metric.is_expired` and `Metric.validate_expires`). These will allow FOG to provide custom validation for its version-based `expires` values.
  * Add a limit of 250 pending ping files. ([mozilla#1217](mozilla/glean#1217)).

Note: This also gets rid of the 2 workarounds (removed code) in
AppService thanks to upstream changes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@thomcc @linabutler