Closed Bug 1404910 Opened 2 years ago Closed 2 years ago

Add tests for bug 1252751

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox-esr52 --- fixed
firefox57 --- fixed
firefox58 --- fixed

People

(Reporter: baku, Assigned: baku)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 2 obsolete files)

When I landed bug 1252751, because it was a security bug, I didn't landed the test patch.
It's time to do it.
Depends on: 1252751
Attached patch ws2.patch (obsolete) — Splinter Review
In order to make this test pass, I have to reintroduce the check on the null principal. I also add an extra |if (!innerWindow) { break; }| but I'm not sure about it.
Attachment #8914346 - Flags: review?(bugs)
Comment on attachment 8914346 [details] [diff] [review]
ws2.patch



>         if (!innerWindow) {
>-          innerWindow = do_QueryInterface(globalObject);
>-          if (NS_WARN_IF(!innerWindow)) {
>-            return NS_ERROR_DOM_SECURITY_ERR;
>-          }
>+          // We are in a sandbox.
>+          break;
>         }
This looks very wrong. We always break here during first iteration since innerWindow is always null there.
Attachment #8914346 - Flags: review?(bugs) → review-
Attached patch ws2.patch (obsolete) — Splinter Review
Ops... that was not what I wanted to submit.
Attachment #8914346 - Attachment is obsolete: true
Attachment #8914484 - Flags: review?(bugs)
Comment on attachment 8914484 [details] [diff] [review]
ws2.patch

>         if (!innerWindow) {
>           innerWindow = do_QueryInterface(globalObject);
>-          if (NS_WARN_IF(!innerWindow)) {
>-            return NS_ERROR_DOM_SECURITY_ERR;
>+          if (!innerWindow) {
>+            // We are in a sandbox.
What sandbox?
Definitely not html sandbox, since there we should have inner window.
Are you talking about the XPConnect sandbox? But we may not be there, since this is used in some random JS component.
Am I missing something here.
Please explain and clarify the comment.
Attachment #8914484 - Flags: review?(bugs) → review-
Attached patch ws2.patchSplinter Review
Attachment #8914484 - Attachment is obsolete: true
Comment on attachment 8914856 [details] [diff] [review]
ws2.patch

Perhaps say that if we are in XPConnect sandbox or in JS component...
(I guess only XPConnect sandbox should have null principal)
Attachment #8914856 - Flags: review+
Duplicate of this bug: 1403328
And please ask approval for the right branches.
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4eb1a4c1fdb1
WebSocket should consider the corrent top-level window principal, r=smaug
Comment on attachment 8914856 [details] [diff] [review]
ws2.patch

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: 
User impact if declined: WebSocket constructor can fail if there is a opener page in https.
Fix Landed on Version: 58
Risk to taking this patch (and alternatives if risky): low. a window/principal check is done in WebSocketInit.
String or UUID changes made by this patch: none

Approval Request Comment
[Feature/Bug causing the regression]: 1347817
[Is this code covered by automated tests?]: yes. tests included.
[Has the fix been verified in Nightly?]: not yet
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: none
Attachment #8914856 - Flags: approval-mozilla-esr52?
Attachment #8914856 - Flags: approval-mozilla-beta?
Comment on attachment 8914856 [details] [diff] [review]
ws2.patch

The other bug was fixed in 48 so I am assuming this is not a new regression. We are trying to minimize risk to 57 quality and schedule by limiting uplifts to the more recent severe regressions due to new features in 57. To me, this fix doesn't meet that bar. If the tests don't need the non-test change, you could land that without relman approval.
Attachment #8914856 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
https://hg.mozilla.org/mozilla-central/rev/4eb1a4c1fdb1
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Note that this patch fixes bug 1403328. This is a regression generated by bug 1347817, landed in 55.
Flags: needinfo?(rkothari)
Hi Baku, I understand. A regression since 55 is still not very recent. The user impact from the beta uplift request is unclear. What would be the user visible impact if the constructor fails here? That might help me reconsider this one. Thanks!
Flags: needinfo?(rkothari)
Hi,
As the original reporter of bug 1403328, which is the regression introduced by bug 1347817 and fixed by this bug, it breaks a real-world application making use of WebSockets in our system in a customer visible manner. We noticed it in early September, but it took me a couple of weeks to understand it well enough to produce a working minimum test case separate from the rest of our system so that I could report it (and include the regression range). I don't know if visible to end-users since August is "not very recent", even if the regression was introduced in nightlies on the 30th of March.

We're notifying customers that this is an upstream bug affecting Firefox stable release versions 55 onward, and are recommending use of alternate browsers (or the ESR 52 release if they're attached to Firefox).

If it's fixed in Firefox 58 (which I understand will be released in mid-January 2018), this means an approximately 5 month window where release versions of the browser are not meaningfully usable for our application. While it was certainly not feasible to expect a fix by release of Firefox 56 (since I reported it the week of release), I would be greatly appreciative if this fix made it into release of Firefox 57 (which I understand will be released in mid-November 2017).

We'll live with it either way, but, there is user impact.

Thanks.
yeah, whatever landed in bug 1347817 is quite wrong and should be fixed everywhere if possible.
Backed out for failing dom/base/test/test_window_cross_origin_props.html with e10s (runs on Windows 7 debug):

https://hg.mozilla.org/mozilla-central/rev/5eba13f5b3a6ad80decdd8c7b30bff5fa477844f

Range containing failing push: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&fromchange=dc2311553356d83b36bb2bc5b27b2ff04f1db6b1&group_state=expanded&filter-searchStr=tc-M%281%29&tochange=fa3f9e9479ae96c92b9a68ca0e8dda2808d9e260
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=134897492&repo=mozilla-inbound
13:25:34     INFO -  2069 INFO None2070 INFO TEST-START | dom/base/test/test_window_cross_origin_props.html
13:25:38     INFO -  GECKO(460) | ++DOMWINDOW == 29 (1BC7B800) [pid = 460] [serial = 2611] [outer = 1EB28C00]
13:25:38     INFO -  GECKO(460) | [460, Main Thread] WARNING: stylo: Web Components not supported yet: file z:/build/build/src/dom/base/nsDocument.cpp, line 6419
13:25:38     INFO -  GECKO(460) | [460, Main Thread] WARNING: stylo: Web Components not supported yet: file z:/build/build/src/dom/base/nsDocument.cpp, line 6419
13:25:38     INFO -  GECKO(460) | ++DOCSHELL 1BE0DC00 == 10 [pid = 460] [id = {25c24ca9-f1a9-4784-a7f8-dee05b07d8e9}]
13:25:38     INFO -  GECKO(460) | ++DOMWINDOW == 30 (1BE0E000) [pid = 460] [serial = 2612] [outer = 00000000]
13:25:38     INFO -  GECKO(460) | ++DOCSHELL 1BE0E400 == 11 [pid = 460] [id = {61f7484b-4e74-49aa-a745-c83e6c6202a9}]
13:25:38     INFO -  GECKO(460) | ++DOMWINDOW == 31 (1BE0E800) [pid = 460] [serial = 2613] [outer = 00000000]
13:25:38     INFO -  GECKO(460) | ++DOMWINDOW == 32 (1BE0EC00) [pid = 460] [serial = 2614] [outer = 1BE0E000]
13:25:38     INFO -  GECKO(460) | ++DOMWINDOW == 33 (1BE10400) [pid = 460] [serial = 2615] [outer = 1BE0E800]
13:25:38     INFO -  GECKO(460) | ++DOMWINDOW == 34 (11F95400) [pid = 460] [serial = 2616] [outer = 1BE0E000]
13:25:38     INFO -  GECKO(460) | [460, Main Thread] WARNING: stylo: Web Components not supported yet: file z:/build/build/src/dom/base/nsDocument.cpp, line 6419
13:25:38     INFO -  GECKO(460) | [460, Main Thread] WARNING: stylo: Web Components not supported yet: file z:/build/build/src/dom/base/nsDocument.cpp, line 6419
13:25:38     INFO -  GECKO(460) | [460, Main Thread] WARNING: stylo: Web Components not supported yet: file z:/build/build/src/dom/base/nsDocument.cpp, line 6419
13:25:38     INFO -  GECKO(460) | [460, Main Thread] WARNING: stylo: Web Components not supported yet: file z:/build/build/src/dom/base/nsDocument.cpp, line 6419
13:25:38     INFO -  GECKO(460) | ++DOMWINDOW == 35 (1BFC7C00) [pid = 460] [serial = 2617] [outer = 1BE0E800]
13:25:38     INFO -  GECKO(460) | [460, Main Thread] WARNING: stylo: Web Components not supported yet: file z:/build/build/src/dom/base/nsDocument.cpp, line 6419
13:25:38     INFO -  GECKO(460) | [460, Main Thread] WARNING: stylo: Web Components not supported yet: file z:/build/build/src/dom/base/nsDocument.cpp, line 6419
13:25:38     INFO -  GECKO(460) | --DOMWINDOW == 34 (11F8F400) [pid = 460] [serial = 2608] [outer = 00000000] [url = http://mochi.test:8888/tests/SimpleTest/iframe-between-tests.html]
13:25:38     INFO -  TEST-INFO | started process screenshot
13:25:38     INFO -  TEST-INFO | screenshot: exit 0
13:25:38     INFO -  Buffered messages logged at 13:25:38
13:25:38     INFO -  2071 INFO Error: Unable to restore focus, expect failures and timeouts.
13:25:38     INFO -  Buffered messages finished
13:25:38    ERROR -  2072 INFO TEST-UNEXPECTED-FAIL | dom/base/test/test_window_cross_origin_props.html | Should have focused first subframe - got "", expected "x"
13:25:38     INFO -      SimpleTest.is@SimpleTest/SimpleTest.js:312:5
13:25:38     INFO -      window.onload@dom/base/test/test_window_cross_origin_props.html:26:5
13:25:38     INFO -      EventHandlerNonNull*@dom/base/test/test_window_cross_origin_props.html:24:3
13:25:38     INFO -  Not taking screenshot here: see the one that was previously logged
13:25:38    ERROR -  2073 INFO TEST-UNEXPECTED-FAIL | dom/base/test/test_window_cross_origin_props.html | Should have focused second subframe - got "", expected "y"
13:25:38     INFO -      SimpleTest.is@SimpleTest/SimpleTest.js:312:5
13:25:38     INFO -      window.onload@dom/base/test/test_window_cross_origin_props.html:29:5
13:25:38     INFO -      EventHandlerNonNull*@dom/base/test/test_window_cross_origin_props.html:24:3
Status: RESOLVED → REOPENED
Flags: needinfo?(amarchesini)
Resolution: FIXED → ---
Target Milestone: mozilla58 → ---
I don't see any relationship between this patch and the failure. Can it be that the tests are running in parallel? But also in this case, there are no WebSockets involved here. The only element in common is that both tests (the failing one and the one included in this patch) use iframes.
Flags: needinfo?(amarchesini)
Looks like the test doesn't close the window it opens.
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c543c0a50db7
WebSocket should consider the corrent top-level window principal, r=smaug
The backout (??) maybe broke linkedin, at least according to mozregression:

https://github.com/webcompat/web-bugs/issues/12367#issuecomment-336238525

baku, does that make any sense to you?

(I suppose we could wait until the changeset in Comment 21 hits Nightly to verify)
Flags: needinfo?(amarchesini)
https://hg.mozilla.org/mozilla-central/rev/c543c0a50db7
Status: REOPENED → RESOLVED
Closed: 2 years ago2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
> baku, does that make any sense to you?

This patch is changing an important the security check for WebSocket. This check was missing in previous versions of firefox. If linkedin works (as I hope it does) with FF57, probably, this bug is unrelated with this webcompact issue.
Flags: needinfo?(amarchesini)
Duplicate of this bug: 1399291
Comment on attachment 8914856 [details] [diff] [review]
ws2.patch

We need this fix for bug 1404093, Beta57+
Attachment #8914856 - Flags: approval-mozilla-beta- → approval-mozilla-beta+
Blocks: 1404093
(In reply to Andrea Marchesini [:baku] from comment #10)
> [Is this code covered by automated tests?]: yes. tests included.
> [Has the fix been verified in Nightly?]: not yet
> [Needs manual test from QE? If yes, steps to reproduce]: no

Setting qe-verify- based on baku's assessment on manual testing needs and the fact that this fix has automated coverage.
Flags: qe-verify-
Comment on attachment 8914856 [details] [diff] [review]
ws2.patch

New tests for ESR52, sounds good to me.
Attachment #8914856 - Flags: approval-mozilla-esr52? → approval-mozilla-esr52+
The bug title here is super confusing. This has nothing to do with tests, but fixing regressions.
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.