1307541 - V4 updates are not scheduled at the right time

Status: RESOLVED FIXED

(Toolkit :: Safe Browsing, defect, P1)

Description

[François Marier [:francois]]

When I enable updates in Nightly, I see the following in the logs:

listmanager: 11:08:43 GMT-0700 (PDT): makeUpdateRequestForEntry_: requestPayload ChUKE25hdmNsaWVudC1hdXRvLWZmb3gaJwgCEAIaGwoNCAIQBhgBIgMwMDEwARDQkAEaAhgHmNXHKyICIAEoARonCAEQAhobCg0IARAGGAEiAzAwMTABEKqTARoCGAfk8cEgIgIgASgBGiYIAxACGhoKDQgDEAYYASIDMDAxMAEQ73waAhgHyJVLpiICIAEoAQ== update: https://safebrowsing.googleapis.com/v4/threatListUpdates:fetch?$ct=application/x-protobuf&key=[trimmed-google-api-key] tablelist: googpub-phish-proto,goog-malware-proto,goog-unwanted-proto

listmanager: 11:08:43 GMT-0700 (PDT): update success for googpub-phish-proto,goog-malware-proto,goog-unwanted-proto from https://safebrowsing.googleapis.com/v4/threatListUpdates:fetch?$ct=application/x-protobuf&key=[trimmed-google-api-key]: 0

listmanager: 11:08:43 GMT-0700 (PDT): Ignoring delay from server (too short), waiting 1800000ms
listmanager: 11:08:43 GMT-0700 (PDT): Setting last update of google4 to 1475604523456
listmanager: 11:08:43 GMT-0700 (PDT): Setting next update of google4 to 1475606323456 (1800000ms from now)

which suggests that we're not parsing the "next update time" ("0" in the second line of the output above) properly in the protobuf response.

Comment 1

[Henry Chang [:hchang]]

Attachment: Bug 1307541 - ProtocolParserProtobuf to init and return update time properly. .

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

Comment 2

[François Marier [:francois]]

Comment on attachment 8797945 [details]
Bug 1307541 - ProtocolParserProtobuf to init and return update time properly. .

https://reviewboard.mozilla.org/r/83534/#review82268

Looks good to me.

I would however suggest that we improve the existing state of things because Thomas just found that the stallness bug was due to a unit conversion mistake.

::: toolkit/components/url-classifier/ProtocolParser.h:43
(Diff revision 1)
>    }
>  
>    nsresult Begin();
>    virtual nsresult AppendStream(const nsACString& aData) = 0;
>  
> +  int32_t UpdateWait() { return mUpdateWait; }

I realize it was like that already, but shouldn't the return value be `uint32_t` to match the member variable?

::: toolkit/components/url-classifier/ProtocolParser.h:78
(Diff revision 1)
>  
>    // The table names that were requested from the client.
>    nsTArray<nsCString> mRequestedTables;
>  
> +  // How long we should wait until the next update.
> +  uint32_t mUpdateWait;

We should specify the unit to avoid conversion errors.

Maybe something like `mUpdateWaitSec` and then changing the comment to:

    // How long we should wait (in seconds) before the next update.

Comment 3

[Henry Chang [:hchang]]

Comment on attachment 8797945 [details]
Bug 1307541 - ProtocolParserProtobuf to init and return update time properly. .

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/83534/diff/1-2/

Comment 4

[François Marier [:francois]]

Comment on attachment 8797945 [details]
Bug 1307541 - ProtocolParserProtobuf to init and return update time properly. .

https://reviewboard.mozilla.org/r/83534/#review82974

I'm confused by the changes you've made in the test file. Did you accidentally commit something you were working on in a different bug?

::: toolkit/components/url-classifier/tests/gtest/TestProtocolParser.cpp:32
(Diff revisions 1 - 2)
>    // for different lists.
>    FetchThreatListUpdatesResponse response;
>  
> +  auto r = response.mutable_list_update_responses()->Add();
> +  InitUpdateResponse(r, SOCIAL_ENGINEERING_PUBLIC,
> +                        nsCString("sta\x00te", 6),

What's the purpose of these `\x00` and `\x0` characters?

Comment 5

[Henry Chang [:hchang]]

Comment on attachment 8797945 [details]
Bug 1307541 - ProtocolParserProtobuf to init and return update time properly. .

https://reviewboard.mozilla.org/r/83534/#review82974

Nope. I just want to make the sample response as real as possible. The previous revision has only the minimum wait duration. This revision is more like a real world data. Besides, the '\x0' is also some sample state/checksum that contains nil char. Thanks :)

Comment 6

[François Marier [:francois]]

Comment on attachment 8797945 [details]
Bug 1307541 - ProtocolParserProtobuf to init and return update time properly. .

https://reviewboard.mozilla.org/r/83534/#review83938

Comment 7

[Henry Chang [:hchang]]

Comment on attachment 8797945 [details]
Bug 1307541 - ProtocolParserProtobuf to init and return update time properly. .

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/83534/diff/2-3/

Comment 8

[Henry Chang [:hchang]]

Comment on attachment 8797945 [details]
Bug 1307541 - ProtocolParserProtobuf to init and return update time properly. .

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/83534/diff/3-4/

Comment 9

[Henry Chang [:hchang]]

Comment on attachment 8797945 [details]
Bug 1307541 - ProtocolParserProtobuf to init and return update time properly. .

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/83534/diff/4-5/

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9bbc15411240
ProtocolParserProtobuf to init and return update time properly. r=francois.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9bbc15411240