requestId should be a string

RESOLVED FIXED in Firefox 48

Status

defect
RESOLVED FIXED
3 years ago
Last year

People

(Reporter: mao, Assigned: TheOne, Mentored)

Tracking

(Blocks 1 bug)

unspecified
mozilla48
Dependency tree / graph

Firefox Tracking Flags

(firefox48 fixed)

Details

(Whiteboard: [webRequest] [good first bug], )

Attachments

(2 attachments, 3 obsolete attachments)

Reporter

Description

3 years ago
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.
Posted patch patch v1 (obsolete) — Splinter Review
Attachment #8730173 - Flags: review?(kmaglione+bmo)
Assignee: nobody → awagner
Status: NEW → ASSIGNED
Posted patch patch v2 (obsolete) — Splinter Review
Fixed eslint errors
Attachment #8730230 - Flags: review?(kmaglione+bmo)
Attachment #8730173 - Attachment is obsolete: true
Attachment #8730173 - Flags: review?(kmaglione+bmo)
Reporter

Comment 4

3 years ago
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/
Reporter

Comment 5

3 years ago
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 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++)`
Attachment #8730230 - Flags: review?(kmaglione+bmo)
Posted patch patch v3 (obsolete) — Splinter Review
Updated according to the comments, the open questions have been discussed and resolved in-person.
Attachment #8730267 - Flags: review?(kmaglione+bmo)
Attachment #8730230 - Attachment is obsolete: true
Attachment #8730267 - Flags: review?(kmaglione+bmo) → review+
Attachment #8730197 - Flags: review?(kmaglione+bmo)
Posted patch patch v4Splinter Review
Sorry for the noise, v4 failed some eslint tests which I didn't realize until after the review.
Attachment #8731113 - Flags: review?(kmaglione+bmo)
Attachment #8730267 - Attachment is obsolete: true
Attachment #8731113 - Flags: review?(kmaglione+bmo) → review+
Keywords: checkin-needed

Comment 12

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/a3ed682cb181
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48

Updated

Last year
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.