Closed Bug 1384658 Opened 7 years ago Closed 7 years ago

Hang with websockets created inside an iframe when page has been navigated to using window.open with _top

Categories

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

55 Branch
All
Windows 10
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox-esr52 --- unaffected
firefox54 --- unaffected
firefox55 + fixed
firefox56 --- fixed

People

(Reporter: marchetti86, Assigned: ehsan.akhgari)

References

Details

(Keywords: hang, regression)

Attachments

(2 files)

Attached image screen_ff.jpg
User Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:55.0) Gecko/20100101 Firefox/55.0
Build ID: 20170724055319

Steps to reproduce:

Enter to this site

http://www.speedy.com.ar/

And click on "Medidor de velocidad" (see screenshot attached if you dont find it)


Actual results:

Firefox cpu goes high and it hangs forever


Expected results:

Should not hang and works good like it does Opera, Chrome and Edge...
I also tried on FF's safe mode but same results...it hangs!
Keywords: hang
Status: UNCONFIRMED → NEW
Has Regression Range: --- → no
Has STR: --- → yes
Ever confirmed: true
Keywords: regression
OS: Unspecified → Windows 10
Hardware: Unspecified → All
Hi Kyle and Ehsan, as Andrea is taking off, could either of you take a look at this since you were the reviewers of bug 1347817? Firefox hang sounds serious.
Flags: needinfo?(kyle)
Flags: needinfo?(ehsan)
Component: Untriaged → DOM
Product: Firefox → Core
I'll take this.
Assignee: nobody → ehsan
Flags: needinfo?(kyle)
Flags: needinfo?(ehsan)
This seems to be more of a regression from bug 1252751. The real issue is that we stuck inside this infinite loop: https://searchfox.org/mozilla-central/rev/bbc1c59e460a27b20929b56489e2e55438de81fa/dom/base/WebSocket.cpp#1652

We start with a principal for http://nexttecnology.speedtestcustom.com/ representing the global object.  innerWindow is 0x7f28c74ce020.  parentWindow is then 0x7f28ea893020 and currentInnerWindow is 0x7f28c8cb0020.  We go to the parent document and enter the next iteration of the outer loop, where we have a principal for http://aplicacion4.speedy.com.ar/medidor-de-velocidad/.  At this point we are at the top of the window hierarchy.  Then we get the opener of the original inner window rather than the opener of the current inner window which seems wrong.  That in this simple case is the parentWindow itself.  So then trivially this assertion would get triggered in a debug build <https://searchfox.org/mozilla-central/rev/bbc1c59e460a27b20929b56489e2e55438de81fa/dom/base/WebSocket.cpp#1699> and in a non-debug build, we would skip over this line as it's ifdef'ed out, line 1702 would be a no-op, and we'd loop indefinitely.

The trap situation is basically like this:

 * Originating page navigates to test page using window.open("test.html", "_top", "").
 * Test page has an iframe called other.com/page.html.
 * other.com/page.html calls new WebSocket("ws://other.com/foo").

It is a mystery to me why bug 1347817 is involved here.  My suspicion is that before that bug, some method was somewhere returning an error code preventing us from getting into this code path, but I didn't really bother building a version before that changeset.
Blocks: 1252751
Summary: Big CPU usage and Firefox Hangs/Stuck (Easy to reproduce) → Hang with websockets created inside an iframe when page has been navigated to using window.open with _top
Attachment #8892262 - Flags: review?(kyle) → review+
Comment on attachment 8892262 [details] [diff] [review]
Correctly account for openers that are the same as the parent window

Approval Request Comment
[Feature/Bug causing the regression]: bug 1347817
[User impact if declined]: hang if the page creates a new WebSocket from an iframe inside a page loaded inside a container page opened using window.open() with _top.
[Is this code covered by automated tests?]: Yes.
[Has the fix been verified in Nightly?]: Not yet.
[Needs manual test from QE? If yes, steps to reproduce]: There are STRs in comment 0. 
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: No, the fix is trivial.
[Why is the change risky/not risky?]: See the patch, this is a simple logic fix.  I think this is even safe on the release channel.
[String changes made/needed]: None.
Attachment #8892262 - Flags: approval-mozilla-release?
Attachment #8892262 - Flags: approval-mozilla-beta?
Pushed by eakhgari@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/566c5e68903a
Correctly account for openers that are the same as the parent window; r=qdot
https://hg.mozilla.org/mozilla-central/rev/566c5e68903a
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
See Also: → 1386290
Tracked for 55, as Julien said, we will try to include this fix in 55.0 RC2, if not it will be in 55.0.1.
Comment on attachment 8892262 [details] [diff] [review]
Correctly account for openers that are the same as the parent window

we need to build 55 rc2, let's fix this websocket hang in there.
Attachment #8892262 - Flags: approval-mozilla-release?
Attachment #8892262 - Flags: approval-mozilla-release+
Attachment #8892262 - Flags: approval-mozilla-beta?
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.