1323856 - Ensure that V4 completions are disabled

Status: RESOLVED FIXED

(Toolkit :: Safe Browsing, defect, P1)

Description

[François Marier [:francois]]

We should set gethashurl for google4 to an empty string and make sure that everything still works (i.e. gethash calls fail and we never honour any partial V4 matches).

This is a fail-safe to ensure that we won't accidentally enable V4 completion ahead of our roll-out schedule.

Comment 1

[Henry Chang [:hchang]]

(In reply to François Marier [:francois] from comment #0)
> We should set gethashurl for google4 to an empty string and make sure that
> everything still works (i.e. gethash calls fail and we never honour any
> partial V4 matches).
> 
> This is a fail-safe to ensure that we won't accidentally enable V4
> completion ahead of our roll-out schedule.

I think

Comment 2

[Henry Chang [:hchang]]

(In reply to Henry Chang [:henry][:hchang] from comment #1)
> (In reply to François Marier [:francois] from comment #0)
> > We should set gethashurl for google4 to an empty string and make sure that
> > everything still works (i.e. gethash calls fail and we never honour any
> > partial V4 matches).
> > 
> > This is a fail-safe to ensure that we won't accidentally enable V4
> > completion ahead of our roll-out schedule.
> 
> I think

Hmm I accidentally left incomplete comment again :(

I am thinking if we should specifically deal with the empty gethash url in the hashcompleter:

Why not: The hashcompleter should have been able to deal with any kind of bad url 
         gracefully. We are supposed to get either an invalid channel or |onStopRequest|
         (as the callback of nsIChannel.asyncOpen2) along with an error code. Either 
         of the cases should be handled very well by the current code. We should just
         leave the code as stupid simple.

Why: The empty gethash url is not only a unreachable url but also a hint 
     that the completion is intentionally disabled. We can catch this
     special url and call back immediately without treating it as an error.

Anyways, before Bug 1312339 is fixed, the v4 gethash url is unlikely to be used because
we haven't added the v4 prefix match results to the LookupResultArray, which is
the input of the hash completion.

Comment 3

[François Marier [:francois]]

(In reply to Henry Chang [:henry][:hchang] from comment #2)
> I am thinking if we should specifically deal with the empty gethash url in
> the hashcompleter:
> 
> Why not: The hashcompleter should have been able to deal with any kind of
> bad url 
>          gracefully. We are supposed to get either an invalid channel or
> |onStopRequest|
>          (as the callback of nsIChannel.asyncOpen2) along with an error
> code. Either 
>          of the cases should be handled very well by the current code. We
> should just
>          leave the code as stupid simple.
> 
> Why: The empty gethash url is not only a unreachable url but also a hint 
>      that the completion is intentionally disabled. We can catch this
>      special url and call back immediately without treating it as an error.

Those are both good points. I would lean towards doing an IsEmpty() check to handle the case where the URL is missing more gracefully. This is the normal case for most of the automated tests.

> Anyways, before Bug 1312339 is fixed, the v4 gethash url is unlikely to be
> used because
> we haven't added the v4 prefix match results to the LookupResultArray, which
> is
> the input of the hash completion.

Excellent.

Comment 4

[Henry Chang [:hchang]]

(In reply to François Marier [:francois] from comment #3)
> (In reply to Henry Chang [:henry][:hchang] from comment #2)
> > I am thinking if we should specifically deal with the empty gethash url in
> > the hashcompleter:
> > 
> > Why not: The hashcompleter should have been able to deal with any kind of
> > bad url 
> >          gracefully. We are supposed to get either an invalid channel or
> > |onStopRequest|
> >          (as the callback of nsIChannel.asyncOpen2) along with an error
> > code. Either 
> >          of the cases should be handled very well by the current code. We
> > should just
> >          leave the code as stupid simple.
> > 
> > Why: The empty gethash url is not only a unreachable url but also a hint 
> >      that the completion is intentionally disabled. We can catch this
> >      special url and call back immediately without treating it as an error.
> 
> Those are both good points. I would lean towards doing an IsEmpty() check to
> handle the case where the URL is missing more gracefully. This is the normal
> case for most of the automated tests.
> 
> > Anyways, before Bug 1312339 is fixed, the v4 gethash url is unlikely to be
> > used because
> > we haven't added the v4 prefix match results to the LookupResultArray, which
> > is
> > the input of the hash completion.
> 
> Excellent.

Just found there is already an emptiness check prior to hash completion [1].
Problem solved!

[1] https://dxr.mozilla.org/mozilla-central/rev/6dbc6e9f62a705d5f523cc750811bd01c8275ec6/toolkit/components/url-classifier/nsUrlClassifierDBService.cpp#963

Comment 5

[François Marier [:francois]]

(In reply to Henry Chang [:henry][:hchang] from comment #4)
> Just found there is already an emptiness check prior to hash completion [1].
> Problem solved!

Great! I guess all we need to do is to clear the pref in modules/libpref/init/all.js.

Comment 6

[Henry Chang [:hchang]]

Attachment: Bug 1323856 - Set v4 gethashURL to empty to avoid accidentally rolling out v4 hash completion.

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

Comment 7

[Henry Chang [:hchang]]

I've just verified the empty v4 gethashURL will not cause any error. Actually I realized comment 4 while verifying the empty URL. The patch should be good to review!

Comment 8

[François Marier [:francois]]

Comment on attachment 8819178 [details]
Bug 1323856 - Set v4 gethashURL to empty to avoid accidentally rolling out v4 hash completion.

https://reviewboard.mozilla.org/r/99024/#review99294

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/527f6a7c1caa
Set v4 gethashURL to empty to avoid accidentally rolling out v4 hash completion. r=francois

Comment 10

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

https://hg.mozilla.org/mozilla-central/rev/527f6a7c1caa