requestId should be a string

RESOLVED FIXED in Firefox 48

Status

WebExtensions
Untriaged
RESOLVED FIXED
2 years ago
5 hours ago

People

(Reporter: mao, Assigned: TheOne, Mentored)

Tracking

(Blocks: 1 bug)

unspecified
mozilla48

Firefox Tracking Flags

(firefox48 fixed)

Details

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

MozReview Requests

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

Attachments

(2 attachments, 3 obsolete attachments)

(Reporter)

Description

2 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.
(Assignee)

Comment 1

2 years ago
Created attachment 8730173 [details] [diff] [review]
patch v1
Attachment #8730173 - Flags: review?(kmaglione+bmo)
(Assignee)

Updated

2 years ago
Assignee: nobody → awagner
Status: NEW → ASSIGNED
(Reporter)

Comment 2

2 years ago
Created attachment 8730197 [details]
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/
Attachment #8730197 - Flags: review?(kmaglione+bmo)
(Assignee)

Comment 3

2 years ago
Created attachment 8730230 [details] [diff] [review]
patch v2

Fixed eslint errors
Attachment #8730230 - Flags: review?(kmaglione+bmo)
(Assignee)

Updated

2 years ago
Attachment #8730173 - Attachment is obsolete: true
Attachment #8730173 - Flags: review?(kmaglione+bmo)
(Reporter)

Comment 4

2 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

2 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)
(Assignee)

Comment 7

2 years ago
Created attachment 8730267 [details] [diff] [review]
patch v3

Updated according to the comments, the open questions have been discussed and resolved in-person.
Attachment #8730267 - Flags: review?(kmaglione+bmo)
(Assignee)

Updated

2 years ago
Attachment #8730230 - Attachment is obsolete: true

Updated

2 years ago
Attachment #8730267 - Flags: review?(kmaglione+bmo) → review+

Updated

2 years ago
Attachment #8730197 - Flags: review?(kmaglione+bmo)
(Assignee)

Comment 10

2 years ago
Created attachment 8731113 [details] [diff] [review]
patch v4

Sorry for the noise, v4 failed some eslint tests which I didn't realize until after the review.
Attachment #8731113 - Flags: review?(kmaglione+bmo)
(Assignee)

Updated

2 years ago
Attachment #8730267 - Attachment is obsolete: true

Updated

2 years ago
Attachment #8731113 - Flags: review?(kmaglione+bmo) → review+
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Comment 12

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/a3ed682cb181
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox48: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48

Updated

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