1404910 - Add tests for bug 1252751

Status: RESOLVED FIXED

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

Description

[Andrea Marchesini [:baku]]

When I landed bug 1252751, because it was a security bug, I didn't landed the test patch.
It's time to do it.

Comment 1

[Andrea Marchesini [:baku]]

Attachment: ws2.patch

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.

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 3

[Andrea Marchesini [:baku]]

Attachment: ws2.patch

Ops... that was not what I wanted to submit.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 5

[Andrea Marchesini [:baku]]

Attachment: ws2.patch

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

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)

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

And please ask approval for the right branches.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4eb1a4c1fdb1
WebSocket should consider the corrent top-level window principal, r=smaug

Comment 10

[Andrea Marchesini [:baku]]

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

Comment 11

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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.

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/4eb1a4c1fdb1

Comment 13

[Andrea Marchesini [:baku]]

Note that this patch fixes bug 1403328. This is a regression generated by bug 1347817, landed in 55.

Comment 14

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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!

Comment 15

[Daniel Selifonov]

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.

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

yeah, whatever landed in bug 1347817 is quite wrong and should be fixed everywhere if possible.

Comment 17

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

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

Comment 19

[Andrea Marchesini [:baku]]

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.

Comment 20

[Olli Pettay [:smaug][[e-mail]]]

Looks like the test doesn't close the window it opens.

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c543c0a50db7
WebSocket should consider the corrent top-level window principal, r=smaug

Comment 22

[Mike Taylor [:miketaylr]]

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)

Comment 23

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

https://hg.mozilla.org/mozilla-central/rev/c543c0a50db7

Comment 24

[Andrea Marchesini [:baku]]

> 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.

Comment 26

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8914856 [details] [diff] [review]
ws2.patch

We need this fix for bug 1404093, Beta57+

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/02b7917df086

Comment 28

[Andrei Vaida [:avaida]]

(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.

Comment 29

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8914856 [details] [diff] [review]
ws2.patch

New tests for ESR52, sounds good to me.

Comment 30

[Olli Pettay [:smaug][[e-mail]]]

The bug title here is super confusing. This has nothing to do with tests, but fixing regressions.

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/5007f2472f64