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

Status: RESOLVED FIXED

(Core :: DOM: Security, defect)

Description

[[:mmc] Monica Chew (no longer reading bugmail)]

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.

Comment 1

[[:mmc] Monica Chew (no longer reading bugmail)]

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

Comment 2

[[:mmc] Monica Chew (no longer reading bugmail)]

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 3

[Gian-Carlo Pascutto [:gcp]]

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?

Comment 4

[[:mmc] Monica Chew (no longer reading bugmail)]

(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.

Comment 5

[[:mmc] Monica Chew (no longer reading bugmail)]

Fixed comment.

https://hg.mozilla.org/integration/mozilla-inbound/rev/aa28e8c20e3a

Comment 6

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

https://hg.mozilla.org/mozilla-central/rev/aa28e8c20e3a

Comment 7

[[:mmc] Monica Chew (no longer reading bugmail)]

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

Comment 8

[[:mmc] Monica Chew (no longer reading bugmail)]

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

Comment 9

[Sylvestre Ledru [:Sylvestre]]

Monica, I guess that does not impact 32, right?

Comment 10

[Ryan VanderMeulen [:RyanVM]]

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 :)

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/898849f49277

Comment 12

[[:mmc] Monica Chew (no longer reading bugmail)]

(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!