V4 updates are not scheduled at the right time

RESOLVED FIXED in Firefox 52

Status

()

P1
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: francois, Assigned: hchang)

Tracking

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

Firefox Tracking Flags

(firefox52 fixed)

Details

(Whiteboard: #sbv4-m0)

Attachments

(1 attachment)

(Reporter)

Description

2 years 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

2 years ago
Assignee: nobody → hchang
Comment hidden (mozreview-request)
(Reporter)

Comment 2

2 years 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

2 years 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

2 years 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

2 years ago
Attachment #8797945 - Flags: review- → review+
(Assignee)

Updated

2 years ago
Attachment #8797945 - Flags: review+ → review?(francois)
(Reporter)

Comment 6

2 years 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

2 years 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

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/9bbc15411240
Status: NEW → RESOLVED
Last Resolved: 2 years 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.