Last Comment Bug 662554 - Make websocketestablishedconnection getinterface and close() check mOwner != nsnull
: Make websocketestablishedconnection getinterface and close() check mOwner != ...
: crash
Product: Core
Classification: Components
Component: Networking: WebSockets (show other bugs)
: 6 Branch
: x86_64 Linux
-- normal (vote)
: mozilla7
Assigned To: Patrick McManus [:mcmanus]
: Patrick McManus [:mcmanus]
: 680957 (view as bug list)
Depends on:
  Show dependency treegraph
Reported: 2011-06-07 09:44 PDT by Patrick McManus [:mcmanus]
Modified: 2011-09-27 10:02 PDT (History)
4 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

662554-websocketestablished-mowner-null.1 (2.97 KB, patch)
2011-06-07 09:46 PDT, Patrick McManus [:mcmanus]
cbiesinger: review+
Details | Diff | Splinter Review

Description User image Patrick McManus [:mcmanus] 2011-06-07 09:44:47 PDT
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.
Comment 1 User image Patrick McManus [:mcmanus] 2011-06-07 09:46:05 PDT
Created attachment 537810 [details] [diff] [review]
Comment 2 User image Christian :Biesinger (don't email me, ping me on IRC) 2011-06-07 13:06:07 PDT
Comment on attachment 537810 [details] [diff] [review]

-    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.
Comment 3 User image Patrick McManus [:mcmanus] 2011-06-08 08:11:31 PDT
Comment 4 User image Patrick McManus [:mcmanus] 2011-08-27 17:16:32 PDT
*** Bug 680957 has been marked as a duplicate of this bug. ***
Comment 5 User image Robert Kaiser 2011-09-21 09:53:47 PDT
Crashes from this have been rising significantly yesterday on 6, adding signatures to have them connected to this bug.
Comment 6 User image Patrick McManus [:mcmanus] 2011-09-27 10:02:55 PDT
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.)

Note You need to log in before you can comment on or make changes to this bug.