Closed Bug 1307541 Opened 6 years ago Closed 6 years ago

V4 updates are not scheduled at the right time

Categories

(Toolkit :: Safe Browsing, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox52 --- fixed

People

(Reporter: francois, Assigned: hchang)

References

Details

(Whiteboard: #sbv4-m0)

Attachments

(1 file)

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.
Assignee: nobody → hchang
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.
Attachment #8797945 - Flags: review?(francois) → review-
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?
Attachment #8797945 - Flags: review?(francois) → review-
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 :)
Attachment #8797945 - Flags: review- → review+
Attachment #8797945 - Flags: review+ → review?(francois)
Comment on attachment 8797945 [details]
Bug 1307541 - ProtocolParserProtobuf to init and return update time properly. .

https://reviewboard.mozilla.org/r/83534/#review83938
Attachment #8797945 - Flags: review?(francois) → review+
Pushed by hchang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9bbc15411240
ProtocolParserProtobuf to init and return update time properly. r=francois.
https://hg.mozilla.org/mozilla-central/rev/9bbc15411240
Status: NEW → RESOLVED
Closed: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in before you can comment on or make changes to this bug.