Closed Bug 1398622 Opened 2 years ago Closed 2 years ago

Services.io.newURI is not a constructor

Categories

(WebExtensions :: Request Handling, defect)

56 Branch
defect
Not set

Tracking

(firefox-esr52 unaffected, firefox55 unaffected, firefox56- fixed, firefox57 fixed)

RESOLVED FIXED
mozilla57
Tracking Status
firefox-esr52 --- unaffected
firefox55 --- unaffected
firefox56 - fixed
firefox57 --- fixed

People

(Reporter: Oriol, Assigned: mixedpuppy)

References

Details

(Keywords: regression)

Attachments

(2 files)

I am seeing this error in the browser console:

    Services.io.newURI is not a constructor
    shouldRunListener resource://gre/modules/WebRequest.jsm:635:13
    runChannelListener resource://gre/modules/WebRequest.jsm:760:14
    observe resource://gre/modules/WebRequest.jsm:579:9

http://searchfox.org/mozilla-central/rev/70cfd6ceecacbe779456654b596bbee4f2b8890b/toolkit/modules/addons/WebRequest.jsm#635

    uri = new Services.io.newURI(`ws${uri.spec.substring(4)}`);
Huh. Don't we have tests for this?
Flags: needinfo?(mixedpuppy)
we do, and they pass. strange.  toolkit/components/extensions/test//mochitest/test_ext_webrequest_websocket.html
Flags: needinfo?(mixedpuppy)
Assignee: nobody → mixedpuppy
ContentPolicyManager fires onBeforeRequest for the ws: url.  We then also get http-on-modify-request for the http version of the ws url, and onBeforeRequest *would* fire again except the stupid error.  The test passes because the first call from ContentPolicyManager.  This is kind of messed up, we shouldn't fire onBeforeRequest twice like this.
[Tracking Requested - why for this release]: Bug exists on beta as well, needs to be fixed.
Comment on attachment 8906471 [details]
Bug 1398622 fix ws handling in contentpolicymanager, and fix stupid error.

https://reviewboard.mozilla.org/r/178226/#review183362
Attachment #8906471 - Flags: review?(kmaglione+bmo) → review+
Pushed by mixedpuppy@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/17b679581392
fix ws handling in contentpolicymanager, and fix stupid error. r=kmag
Track 56- as this is already in regression triage list and happy to take the patch in 56.
https://hg.mozilla.org/mozilla-central/rev/17b679581392
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Please request Beta approval on this when you get a chance.
Flags: needinfo?(mixedpuppy)
Flags: in-testsuite+
Comment on attachment 8906471 [details]
Bug 1398622 fix ws handling in contentpolicymanager, and fix stupid error.

Approval Request Comment
[Feature/Bug causing the regression]: 1382834
[User impact if declined]: websocket requests are not correctly handled in webextension webrequest
[Is this code covered by automated tests?]: Yes.  Part of this patch removes a bug that obscured this coding error from the existing tests.
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: low
[Why is the change risky/not risky?]: fix is relatively trivial
[String changes made/needed]:
Flags: needinfo?(mixedpuppy)
Attachment #8906471 - Flags: approval-mozilla-beta?
Comment on attachment 8906471 [details]
Bug 1398622 fix ws handling in contentpolicymanager, and fix stupid error.

Fix for websocket regression, please uplift for beta 12.
Attachment #8906471 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8907905 - Flags: review?(kmaglione+bmo)
Comment on attachment 8907905 [details] [diff] [review]
beta fix for policyType

got the + on irc.
Attachment #8907905 - Flags: review?(kmaglione+bmo) → review+
Is manual testing required on this bug? If yes, please provide some STR and the proper webextension(if required) or set the “qe-verify-“ flag.
Flags: needinfo?(mixedpuppy)
Flags: needinfo?(mixedpuppy) → qe-verify-
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.