Closed Bug 1041855 Opened 5 years ago Closed 5 years ago

Skip gethash completions on startup for hits in existing tables that haven't been registered in listmanager yet

Categories

(Core :: DOM: Security, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla34
Tracking Status
firefox32 --- unaffected
firefox33 --- fixed
firefox34 --- fixed

People

(Reporter: mmc, Assigned: mmc)

Details

Attachments

(1 file, 1 obsolete file)

Otherwise, if the list manager is not done registering tables and the DB service finds a hit in a table on disk, we'll break in the DB service because the gethash URL is empty for a non-test table.
Assignee: nobody → mmc
Status: NEW → ASSIGNED
Attachment #8460644 - Flags: review?(gpascutto)
Summary: nsIListManager.getGethashUrl should throw instead of returning "" if no gethash URL is available → Skip gethash completions on startup for hits in existing tables that haven't been registered in listmanager yet
Btw, the original bug summary (make listmanager throw instead of returning empty gethash) doesn't work because for test tables we don't set gethash urls, and it would break our tests.
Comment on attachment 8460644 [details] [diff] [review]
Skip gethash completions on startup for hits in existing tables that haven't been registered yet (

Review of attachment 8460644 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/url-classifier/nsUrlClassifierDBService.cpp
@@ +836,5 @@
>          "@mozilla.org/url-classifier/listmanager;1", &rv);
>        NS_ENSURE_SUCCESS(rv, rv);
>        rv = listManager->GetGethashUrl(result.mTableName, gethashUrl);
>        NS_ENSURE_SUCCESS(rv, rv);
> +      // When a completer exists, only allow empty gethashUrls for test tables.

This comment looks out of date.

@@ +840,5 @@
> +      // When a completer exists, only allow empty gethashUrls for test tables.
> +      // Otherwise, we may have found a prefix in an existing table on startup
> +      // before the listmanager has registered the table yet, in which case we
> +      // should not call complete.
> +      if ((!gethashUrl.IsEmpty() ||

We just went from "assert this never happens because we can't cope with it if it did" to "oops this happens and we need to deal with it", right?
Attachment #8460644 - Flags: review?(gpascutto) → review+
(In reply to Gian-Carlo Pascutto [:gcp] from comment #3)
> Comment on attachment 8460644 [details] [diff] [review]
> Skip gethash completions on startup for hits in existing tables that haven't
> been registered yet (
> 
> Review of attachment 8460644 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/components/url-classifier/nsUrlClassifierDBService.cpp
> @@ +836,5 @@
> >          "@mozilla.org/url-classifier/listmanager;1", &rv);
> >        NS_ENSURE_SUCCESS(rv, rv);
> >        rv = listManager->GetGethashUrl(result.mTableName, gethashUrl);
> >        NS_ENSURE_SUCCESS(rv, rv);
> > +      // When a completer exists, only allow empty gethashUrls for test tables.
> 
> This comment looks out of date.

Will fix.
 
> @@ +840,5 @@
> > +      // When a completer exists, only allow empty gethashUrls for test tables.
> > +      // Otherwise, we may have found a prefix in an existing table on startup
> > +      // before the listmanager has registered the table yet, in which case we
> > +      // should not call complete.
> > +      if ((!gethashUrl.IsEmpty() ||
> 
> We just went from "assert this never happens because we can't cope with it
> if it did" to "oops this happens and we need to deal with it", right?

Yes. There are two cases for empty gethash
1) test tables
2) production tables before listmanager startup. We can't call complete in this case.

Actually, maybe that's a better comment than my initial one.
https://hg.mozilla.org/mozilla-central/rev/aa28e8c20e3a
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
Comment on attachment 8465566 [details] [diff] [review]
Skip gethash completions on startup for hits in existing tables that haven't been registered yet (

Approval Request Comment

[Feature/regressing bug #]: bug 1021419

[User impact if declined]: On rare occasions, if the user visits a phishing/malware page on startup before the list manager is done registering tables, the browser will crash with the assertion that was removed. Previously the gethashURL was not configurable per table and the DBService was less dependent on listmanager startup.

[Describe test coverage new/current, TBPL]: manually testing that visiting phishing page immediately after startup does not crash.

[Risks and why]: Pretty low risk.

[String/UUID change made/needed]: None
Attachment #8465566 - Flags: approval-mozilla-aurora?
Attachment #8465566 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Monica, I guess that does not impact 32, right?
Comment on attachment 8460644 [details] [diff] [review]
Skip gethash completions on startup for hits in existing tables that haven't been registered yet (

Please mark obsolete patches as such :)
Attachment #8460644 - Attachment is obsolete: true
(In reply to Sylvestre Ledru [:sylvestre] from comment #9)
> Monica, I guess that does not impact 32, right?

Nope, it is unaffected. Thanks Ryan and Sylvestre!
You need to log in before you can comment on or make changes to this bug.