If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

V4 updates are not scheduled at the right time

RESOLVED FIXED in Firefox 52

Status

()

Toolkit
Safe Browsing
P1
normal
RESOLVED FIXED
a year ago
11 months ago

People

(Reporter: francois, Assigned: hchang)

Tracking

(Blocks: 1 bug)

unspecified
mozilla52
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox52 fixed)

Details

(Whiteboard: #sbv4-m0)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Reporter)

Description

a year ago
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)

Updated

a year ago
Assignee: nobody → hchang
Comment hidden (mozreview-request)
(Reporter)

Comment 2

a year ago
mozreview-review
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 hidden (mozreview-request)
(Reporter)

Comment 4

a year ago
mozreview-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-
(Assignee)

Comment 5

a year ago
mozreview-review-reply
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 :)
(Assignee)

Updated

a year ago
Attachment #8797945 - Flags: review- → review+
(Assignee)

Updated

a year ago
Attachment #8797945 - Flags: review+ → review?(francois)
(Reporter)

Comment 6

11 months ago
mozreview-review
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+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 10

11 months ago
Pushed by hchang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9bbc15411240
ProtocolParserProtobuf to init and return update time properly. r=francois.

Comment 11

11 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/9bbc15411240
Status: NEW → RESOLVED
Last Resolved: 11 months ago
status-firefox52: --- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in before you can comment on or make changes to this bug.