1256264 - requestId should be a string

Status: RESOLVED FIXED

(WebExtensions :: Untriaged, defect)

Description

[Giorgio Maone [:ma1]]

We've finally got requestId passed on every WebRequest event, but we pass an integer while in Chrome string (which appears still to be a monotonously increment integer as weel, just stringified) gets passed.
I can see the rationale behind it: if lots and lots (frankly, too many for a lifetime-long browser session) of requests are initiated, we might exhaust the 54 bits available for precise integer representation and start skipping ids, at some point in the next century or so. Hence, bonus point if you manage to make this integer potentially unlimited (i.e. a big integer), but I'd consider this fixed if we just achieve chrome compatibility by stringifying the id as it's fetched for the first time, i.e. in the RequestId.create() object.

Comment 1

[Andreas Wagner [:TheOne] [use NI]]

Attachment: patch v1

Comment 2

[Giorgio Maone [:ma1]]

Attachment: MozReview Request: Bug 1256264 Make requestId a string

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

Comment 3

[Andreas Wagner [:TheOne] [use NI]]

Attachment: patch v2

Fixed eslint errors

Comment 4

[Giorgio Maone [:ma1]]

Comment on attachment 8730197 [details]
MozReview Request: Bug 1256264 Make requestId a string

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

Comment 5

[Giorgio Maone [:ma1]]

Comment on attachment 8730197 [details]
MozReview Request: Bug 1256264 Make requestId a string

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

Comment 6

[Kris Maglione [:kmag]]

Comment on attachment 8730230 [details] [diff] [review]
patch v2

Review of attachment 8730230 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/extensions/test/mochitest/test_ext_webrequest.html
@@ +250,5 @@
>  
> +  let lastRequestId = -1;
> +  let lastRequestUrl = null;
> +  function validateRequestIdType(currentId) {
> +    browser.test.assertTrue(typeof lastRequestId === "string");

`browser.test.assertEq("string", typeof(lastRequestId), ...)` Same for the tests below.

Please also include a description of each test, or the test output is going to be difficult to follow.

@@ +252,5 @@
> +  let lastRequestUrl = null;
> +  function validateRequestIdType(currentId) {
> +    browser.test.assertTrue(typeof lastRequestId === "string");
> +    browser.test.assertTrue(typeof currentId === "string");
> +    browser.test.assertTrue(typeof parseInt(currentId, 10) === "number");

This will always be true. Do you mean `!isNaN(...)`?

@@ +253,5 @@
> +  function validateRequestIdType(currentId) {
> +    browser.test.assertTrue(typeof lastRequestId === "string");
> +    browser.test.assertTrue(typeof currentId === "string");
> +    browser.test.assertTrue(typeof parseInt(currentId, 10) === "number");
> +    browser.test.assertTrue(parseInt(lastRequestId, 10) !== parseInt(currentId, 10));

Why do you need parseInt here?

@@ +264,5 @@
> +      lastRequestUrl = details.url;
> +      lastRequestId = details.requestId;
> +    } else if (lastRequestUrl != details.url) {
> +      validateRequestIdType(details.requestId);
> +    }

Why these changes?

::: toolkit/modules/addons/WebRequest.jsm
@@ +43,5 @@
>  var RequestId = {
>    count: 1,
>    KEY: "mozilla.webRequest.requestId",
>    create(channel = null) {
> +    let id = (this.count++).toString();

`String(this.count++)`

Comment 7

[Andreas Wagner [:TheOne] [use NI]]

Attachment: patch v3

Updated according to the comments, the open questions have been discussed and resolved in-person.

Comment 8

[Andreas Wagner [:TheOne] [use NI]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=00a2106fa9b6

Comment 9

[Andreas Wagner [:TheOne] [use NI]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=cf090bbe236f

Comment 10

[Andreas Wagner [:TheOne] [use NI]]

Attachment: patch v4

Sorry for the noise, v4 failed some eslint tests which I didn't realize until after the review.

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/a3ed682cb181

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a3ed682cb181