Last Comment Bug 662554 - Make websocketestablishedconnection getinterface and close() check mOwner != nsnull
: Make websocketestablishedconnection getinterface and close() check mOwner != ...
Status: RESOLVED FIXED
: crash
Product: Core
Classification: Components
Component: Networking: WebSockets (show other bugs)
: 6 Branch
: x86_64 Linux
: -- normal (vote)
: mozilla7
Assigned To: Patrick McManus [:mcmanus]
:
Mentors:
: 680957 (view as bug list)
Depends on:
Blocks:
  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: ---


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

Description 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 Patrick McManus [:mcmanus] 2011-06-07 09:46:05 PDT
Created attachment 537810 [details] [diff] [review]
662554-websocketestablished-mowner-null.1
Comment 2 Christian :Biesinger (don't email me, ping me on IRC) 2011-06-07 13:06:07 PDT
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.
Comment 3 Patrick McManus [:mcmanus] 2011-06-08 08:11:31 PDT
http://hg.mozilla.org/mozilla-central/rev/b9354cf9282d
Comment 4 Patrick McManus [:mcmanus] 2011-08-27 17:16:32 PDT
*** Bug 680957 has been marked as a duplicate of this bug. ***
Comment 5 Robert Kaiser (not working on stability any more) 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 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.