Last Comment Bug 758224 - bug in nsPluginStreamListenerPeer dtor, pushing nearly-invalid address to an outside reference
: bug in nsPluginStreamListenerPeer dtor, pushing nearly-invalid address to an ...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Plug-ins (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla15
Assigned To: Josh Aas
:
: Benjamin Smedberg [:bsmedberg]
Mentors:
Depends on:
Blocks: 758227
  Show dependency treegraph
 
Reported: 2012-05-24 07:53 PDT by Josh Aas
Modified: 2012-05-30 13:50 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
fix v1.0 (880 bytes, patch)
2012-05-24 07:53 PDT, Josh Aas
benjamin: review+
akeybl: approval‑mozilla‑aurora-
Details | Diff | Splinter Review

Description Josh Aas 2012-05-24 07:53:54 PDT
Created attachment 626807 [details] [diff] [review]
fix v1.0

294 nsPluginStreamListenerPeer::~nsPluginStreamListenerPeer()
295 {
296 #ifdef PLUGIN_LOGGING
297   PR_LOG(nsPluginLogging::gPluginLog, PLUGIN_LOG_NORMAL,
298          ("nsPluginStreamListenerPeer::dtor this=%p, url=%s\n",this, mURLSpec.get()));
299 #endif
300 
301   if (mPStreamListener) {
302     mPStreamListener->SetStreamListenerPeer(this);
303   }

We should be doing 'SetStreamListenerPeer(nsnull)', not setting it to 'this' from the dtor. Found this while writing another patch that exposes this bug via double-free.
Comment 1 Josh Aas 2012-05-24 08:00:13 PDT
pushed to mozilla-inbound

http://hg.mozilla.org/integration/mozilla-inbound/rev/3a519ed51c90
Comment 2 Josh Aas 2012-05-24 10:21:48 PDT
Comment on attachment 626807 [details] [diff] [review]
fix v1.0

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 475991
User impact if declined: Possible cause of crashes.
Testing completed (on m-c, etc.): Should land on m-c today.
Risk to taking this patch (and alternatives if risky): Not risky.
String or UUID changes made by this patch: None
Comment 3 :Ms2ger (⌚ UTC+1/+2) 2012-05-25 08:29:13 PDT
https://hg.mozilla.org/mozilla-central/rev/3a519ed51c90
Comment 4 Alex Keybl [:akeybl] 2012-05-29 11:06:44 PDT
(In reply to Josh Aas (Mozilla Corporation) from comment #2)
> Comment on attachment 626807 [details] [diff] [review]
> fix v1.0
> 
> [Approval Request Comment]
> Bug caused by (feature/regressing bug #): Bug 475991
> User impact if declined: Possible cause of crashes.
> Testing completed (on m-c, etc.): Should land on m-c today.
> Risk to taking this patch (and alternatives if risky): Not risky.
> String or UUID changes made by this patch: None

Do we have an idea of speculative volume of crashes this would fix? If thought to be low, I think this can ride the trains given how old the regressing Bug 475991 is.
Comment 5 Josh Aas 2012-05-30 09:12:40 PDT
(In reply to Alex Keybl [:akeybl] from comment #4)

> Do we have an idea of speculative volume of crashes this would fix? If
> thought to be low, I think this can ride the trains given how old the
> regressing Bug 475991 is.

I don't know for sure because it's not clear what the crash signature might be. I think the crash volume is probably low, possibly non-existent.

The patch is very low risk though, and it's the type of bug that concerns me quite a bit (promoting usage of a pointer to a deleted object).

The crash would probably happen here if it happened at all:

http://mxr.mozilla.org/mozilla-central/source/dom/plugins/base/nsNPAPIPlugin.cpp#541
Comment 6 Alex Keybl [:akeybl] 2012-05-30 13:50:14 PDT
Comment on attachment 626807 [details] [diff] [review]
fix v1.0

[Triage Comment]
Let's let this ride the trains if it's unclear how much impact this patch will have.

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