Replace manual refcounting of nsTimeouts in nsGlobalWindow.cpp with equialent nsRefPtr usage

RESOLVED FIXED in mozilla7

Status

()

Core
General
--
minor
RESOLVED FIXED
8 years ago
6 years ago

People

(Reporter: bnewman, Assigned: Wade Smith)

Tracking

Trunk
mozilla7
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

8 years ago
I suspect there may be bugs in all the calls to timeout->AddRef() and timeout->Release() found in nsGlobalWindow.cpp.  For better or worse, most of them are meticulously commented, e.g.

  // Hold on to the timeout to ensure it doesn't go away while it's
  // being handled (aka kungFuDeathGrip).
  timeout->AddRef();
  timeout->mWindow->RunTimeout(timeout);
  // Drop our reference to the timeout now that we're done with it.
  timeout->Release();

This kind of discipline is unnecessary and error-prone.  Almost all of these usages can be replaced with nsRefPtr stack-based auto-refcounting.
(Reporter)

Updated

8 years ago
Summary: Replace manual refcounting of nsTimeouts in nsGlobalWindow.cpp with equialent nsRefPtr → Replace manual refcounting of nsTimeouts in nsGlobalWindow.cpp with equialent nsRefPtr usage
Whiteboard: [good first bug]
(Assignee)

Comment 1

6 years ago
Created attachment 538173 [details] [diff] [review]
Patch to replace a couple of manual ref-countings with automatic

This is the first patch I've ever written, so let me know if I've done something wrong so I can learn :). There were a couple of instances of calls to AddRef() that didn't seem like they could be removed, but there were two places where AddRef() was called and then Release() called at every point the function returned which have been removed and replaced with an nsRefPtr

Comment 2

6 years ago
Hey, this is great! Thanks! Let's see what jst thinks.

Updated

6 years ago
Attachment #538173 - Flags: review?(jst)
Comment on attachment 538173 [details] [diff] [review]
Patch to replace a couple of manual ref-countings with automatic

Yup, this looks good to me. Thanks Wade!
Attachment #538173 - Flags: review?(jst) → review+

Updated

6 years ago
Keywords: checkin-needed

Updated

6 years ago
Assignee: nobody → wade.cameron
http://hg.mozilla.org/integration/mozilla-inbound/rev/2cc9093cdd23
Keywords: checkin-needed
Whiteboard: [good first bug] → [inbound]
Merged:
http://hg.mozilla.org/mozilla-central/rev/2cc9093cdd23
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Whiteboard: [inbound]
Target Milestone: --- → mozilla7
Version: unspecified → Trunk

Updated

6 years ago
Duplicate of this bug: 395563
You need to log in before you can comment on or make changes to this bug.