runChannelListener is still too complex.

RESOLVED FIXED in Firefox 52

Status

()

Toolkit
WebExtensions: Request Handling
RESOLVED FIXED
a year ago
11 months ago

People

(Reporter: kmag, Assigned: kmag)

Tracking

51 Branch
mozilla52
Points:
---

Firefox Tracking Flags

(firefox52 fixed)

Details

MozReview Requests

()

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

Attachments

(1 attachment)

At least according to ESLint. And I'm inclined to agree.
Comment hidden (mozreview-request)

Comment 2

a year ago
mozreview-review
Comment on attachment 8809896 [details]
Bug 1316914: Refactor runChannelListener to decrease complexity to a manageable level.

https://reviewboard.mozilla.org/r/92388/#review92460

::: toolkit/modules/addons/WebRequest.jsm:612
(Diff revision 1)
> -        }
> -
> -        if (!commonData) {
> -          commonData = {
> -            requestId: RequestId.get(channel),
> +      requestId: RequestId.get(channel),
> -            url: uri.spec,
> +      url: channel.URI.spec.spec,

is that right?  .spec.spec?

::: toolkit/modules/addons/WebRequest.jsm:630
(Diff revision 1)
> -            }
> +      }
>  
> -            let {isSystemPrincipal} = Services.scriptSecurityManager;
> +      let {isSystemPrincipal} = Services.scriptSecurityManager;
>  
> -            commonData.isSystemPrincipal = (isSystemPrincipal(loadInfo.triggeringPrincipal) ||
> +      data.isSystemPrincipal = (isSystemPrincipal(loadInfo.triggeringPrincipal) ||
> -                                            isSystemPrincipal(loadInfo.loadingPrincipal));
> +                                isSystemPrincipal(loadInfo.loadingPrincipal));

If triggeringPrincipal is supposed to always be defined, do we need to check laodingPrincipal any longer?
(Assignee)

Comment 3

a year ago
mozreview-review-reply
Comment on attachment 8809896 [details]
Bug 1316914: Refactor runChannelListener to decrease complexity to a manageable level.

https://reviewboard.mozilla.org/r/92388/#review92460

> is that right?  .spec.spec?

Nope. I think I accidentally hit '.' before I saved :/

> If triggeringPrincipal is supposed to always be defined, do we need to check laodingPrincipal any longer?

`loadingPrincipal` and `triggeringPrincipal` are required to be defined, and that's enforced by assertions in Necko code. If `triggeringPrincipal` is not provided, it automatically falls back to `loadingPrincipal`.

Comment 4

a year ago
mozreview-review
Comment on attachment 8809896 [details]
Bug 1316914: Refactor runChannelListener to decrease complexity to a manageable level.

https://reviewboard.mozilla.org/r/92386/#review92478

Fix the spec issue, then go for it.

Comment 5

a year ago
mozreview-review
Comment on attachment 8809896 [details]
Bug 1316914: Refactor runChannelListener to decrease complexity to a manageable level.

https://reviewboard.mozilla.org/r/92386/#review92480

Comment 6

a year ago
mozreview-review
Comment on attachment 8809896 [details]
Bug 1316914: Refactor runChannelListener to decrease complexity to a manageable level.

https://reviewboard.mozilla.org/r/92388/#review92488

per last comment, fix spec, then r+
Attachment #8809896 - Flags: review?(mixedpuppy) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/67858d51f777ad68685a7d0cca0355229fe17716
Bug 1316914: Refactor runChannelListener to decrease complexity to a manageable level. r=mixedpuppy
Backed out bug 1316914, bug 1316784 and bug 1305217 for frequent timeouts in test_ext_webrequest_upload.html:

Bug 1305217:
https://hg.mozilla.org/integration/mozilla-inbound/rev/217ae1d5285328611de99e2e48042b19751dbb54
https://hg.mozilla.org/integration/mozilla-inbound/rev/6cbde9c5e058999ba95319391b59a7d67649ca53

Bug 1316784:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9381a00d19f81c21114f9d24cafee791fa0d9dbe

Bug 1316914:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ddec56f2c469512ba99e424540f6f77a447fdea0

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=39104153&repo=mozilla-inbound

21:16:58     INFO - TEST-PASS | toolkit/components/extensions/test/mochitest/test_ext_webrequest_upload.html | Binary upload size matches - Expected: 16, Actual: 16 
21:16:58     INFO - onCompleted 32 http://mochi.test:8888/tests/toolkit/components/extensions/test/mochitest/file_WebRequest_page3.html?trigger=form&upload=16+bytes&enctype=multipart%2Fform-data&xhr=1
21:16:58     INFO - onBeforeRequest 33 http://mochi.test:8888/tests/toolkit/components/extensions/test/mochitest/file_WebRequest_page3.html?trigger=form&upload=%7B%22textInput%22%3A%5B%22value1%22%2C%22value2%22%5D%7D&enctype=application%2Fx-www-form-urlencoded
21:16:58     INFO - onUpload http://mochi.test:8888/tests/toolkit/components/extensions/test/mochitest/file_WebRequest_page3.html?trigger=form&upload=%7B%22textInput%22%3A%5B%22value1%22%2C%22value2%22%5D%7D&enctype=application%2Fx-www-form-urlencoded {"formData":{"textInput":["value1","value2"]}}
21:16:58     INFO - TEST-PASS | toolkit/components/extensions/test/mochitest/test_ext_webrequest_upload.html | Intercepted upload http://mochi.test:8888/tests/toolkit/components/extensions/test/mochitest/file_WebRequest_page3.html?trigger=form&upload=%7B%22textInput%22%3A%5B%22value1%22%2C%22value2%22%5D%7D&enctype=application%2Fx-www-form-urlencoded #33 {"textInput":["value1","value2"]} have a requestBody 
21:16:58     INFO - TEST-PASS | toolkit/components/extensions/test/mochitest/test_ext_webrequest_upload.html | Upload http://mochi.test:8888/tests/toolkit/components/extensions/test/mochitest/file_WebRequest_page3.html?trigger=form&upload=%7B%22textInput%22%3A%5B%22value1%22%2C%22value2%22%5D%7D&enctype=application%2Fx-www-form-urlencoded #33 matches form data. - Expected: {"textInput":["value1","value2"]}, Actual: {"textInput":["value1","value2"]} 
21:16:58     INFO - Buffered messages finished
21:16:58     INFO - TEST-UNEXPECTED-FAIL | toolkit/components/extensions/test/mochitest/test_ext_webrequest_upload.html | Test timed out. 
21:16:58     INFO -     reportError@SimpleTest/TestRunner.js:114:7
21:16:58     INFO -     TestRunner._checkForHangs@SimpleTest/TestRunner.js:135:7
Flags: needinfo?(kmaglione+bmo)
https://hg.mozilla.org/integration/mozilla-inbound/rev/9220ac85e9f1789cb1a41abe23946b31080a86a9
Bug 1316914: Refactor runChannelListener to decrease complexity to a manageable level. r=mixedpuppy
Flags: needinfo?(kmaglione+bmo)

Comment 10

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