1316914 - runChannelListener is still too complex.

Status: RESOLVED FIXED

(WebExtensions :: Request Handling, defect)

Description

[Kris Maglione [:kmag]]

At least according to ESLint. And I'm inclined to agree.

Comment 1

[Kris Maglione [:kmag]]

Attachment: Bug 1316914: Refactor runChannelListener to decrease complexity to a manageable level.

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

Comment 2

[Shane Caraveo (:mixedpuppy)]

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?

Comment 3

[Kris Maglione [:kmag]]

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

[Shane Caraveo (:mixedpuppy)]

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

[Shane Caraveo (:mixedpuppy)]

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

[Shane Caraveo (:mixedpuppy)]

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+

Comment 7

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/67858d51f777ad68685a7d0cca0355229fe17716
Bug 1316914: Refactor runChannelListener to decrease complexity to a manageable level. r=mixedpuppy

Comment 8

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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

Comment 9

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9220ac85e9f1789cb1a41abe23946b31080a86a9
Bug 1316914: Refactor runChannelListener to decrease complexity to a manageable level. r=mixedpuppy

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9220ac85e9f1