Make websocketestablishedconnection getinterface and close() check mOwner != nsnull

RESOLVED FIXED in mozilla7

Status

()

Core
Networking: WebSockets
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: mcmanus, Assigned: mcmanus)

Tracking

({crash})

6 Branch
mozilla7
x86_64
Linux
crash
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(crash signature)

Attachments

(1 attachment)

(Assignee)

Description

6 years ago
This is from a code audit I was doing - it seems possible for Disconnect() to be called from perhaps NS_ENSURE_SUCCESS_AND_FAIL* or something like that, and then Close() to be called when the page is unloaded.. in which case mOwner will be deref'd null in the close() function. I haven't been able to make it happen, though.

in any event all the other WSEC methods check mOwner, so it makes sense to do so in the two places where it isn't: close() and getinterface(). I also added a couple NS_ABORT_IF_FALSE(mOwner) calls in a couple places to define that as a precondition (which I verified is met) for calling those functions.
(Assignee)

Comment 1

6 years ago
Created attachment 537810 [details] [diff] [review]
662554-websocketestablished-mowner-null.1
Assignee: nobody → mcmanus
Status: NEW → ASSIGNED
Attachment #537810 - Flags: review?(cbiesinger)
Comment on attachment 537810 [details] [diff] [review]
662554-websocketestablished-mowner-null.1

-    nsCOMPtr<nsIDocument> doc =
-      nsContentUtils::GetDocumentFromScriptContext(mOwner->mScriptContext);
+    nsCOMPtr<nsIDocument> doc;
+    if (mOwner)
+      doc = nsContentUtils::GetDocumentFromScriptContext(
+        mOwner->mScriptContext);

since this code just returns failure for a null doc, I'd just add an if (!mOwner) return NS_ERROR_FAILURE; before this block and avoid the additional indentation level for this line.
Attachment #537810 - Flags: review?(cbiesinger) → review+
(Assignee)

Comment 3

6 years ago
http://hg.mozilla.org/mozilla-central/rev/b9354cf9282d
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
(Assignee)

Updated

6 years ago
Target Milestone: --- → mozilla7
(Assignee)

Updated

6 years ago
Duplicate of this bug: 680957

Comment 5

6 years ago
Crashes from this have been rising significantly yesterday on 6, adding signatures to have them connected to this bug.
Crash Signature: [@ nsWebSocketEstablishedConnection::Close() ] [@ nsWebSocketEstablishedConnection::Close ]

Updated

6 years ago
Keywords: crash, regression
Version: unspecified → 6 Branch
(Assignee)

Comment 6

6 years ago
I removed the regression keyword lacking evidence of something in particular that regressed. (I think this was the first release of websockets and it contained a bug limited to websockets.)
Keywords: regression
You need to log in before you can comment on or make changes to this bug.