Closed Bug 1123004 Opened 5 years ago Closed 5 years ago

Mark ReadSegmentsState as stack class, and its mThisStream as nsCOMPtr; r=froydnj

Categories

(Core :: XPCOM, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla38

People

(Reporter: ehsan, Assigned: ehsan)

Details

Attachments

(1 file)

No description provided.
Assignee: nobody → ehsan
Comment on attachment 8550810 [details] [diff] [review]
Mark ReadSegmentsState as stack class, and its mThisStream as nsCOMPtr

Review of attachment 8550810 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with reservations; some sort of discussion of the below concern would be good before this gets checked in.

::: netwerk/base/src/nsMIMEInputStream.cpp
@@ +45,5 @@
>  
>      void InitStreams();
>  
> +    struct MOZ_STACK_CLASS ReadSegmentsState {
> +        nsCOMPtr<nsIInputStream> mThisStream;

I'm a little skeptical of the goodness of the nsCOMPtr usage in this patch (and other, similar patches); we're addref'ing |this|, which seems unlikely to go away while we're reading from the stream.  (I realize the same sort of argument is not as applicable to something like DOM code...)  What's your rationale for adding strong references, rather than annotating?

::: xpcom/io/nsMultiplexInputStream.cpp
@@ +50,2 @@
>    {
> +    nsCOMPtr<nsIInputStream> mThisStream;

Here too.
(In reply to Nathan Froyd [:froydnj] [:nfroyd] from comment #2)
> Comment on attachment 8550810 [details] [diff] [review]
> Mark ReadSegmentsState as stack class, and its mThisStream as nsCOMPtr
> 
> Review of attachment 8550810 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me with reservations; some sort of discussion of the below concern would
> be good before this gets checked in.
> 
> ::: netwerk/base/src/nsMIMEInputStream.cpp
> @@ +45,5 @@
> >  
> >      void InitStreams();
> >  
> > +    struct MOZ_STACK_CLASS ReadSegmentsState {
> > +        nsCOMPtr<nsIInputStream> mThisStream;
> 
> I'm a little skeptical of the goodness of the nsCOMPtr usage in this patch
> (and other, similar patches); we're addref'ing |this|, which seems unlikely
> to go away while we're reading from the stream.  (I realize the same sort of
> argument is not as applicable to something like DOM code...)  What's your
> rationale for adding strong references, rather than annotating?
> 
> ::: xpcom/io/nsMultiplexInputStream.cpp
> @@ +50,2 @@
> >    {
> > +    nsCOMPtr<nsIInputStream> mThisStream;
> 
> Here too.

Mostly to bullet proof future code.  I think in cases where there are no performance concerns, we should prefer strong references.  It's hard to keep invariants valid otherwise.  WDYT?
Flags: needinfo?(nfroyd)
(In reply to :Ehsan Akhgari [Away: 1/29-2/20] (not reading bugmail, needinfo? me!) from comment #3)
> (In reply to Nathan Froyd [:froydnj] [:nfroyd] from comment #2)
> > ::: netwerk/base/src/nsMIMEInputStream.cpp
> > @@ +45,5 @@
> > >  
> > >      void InitStreams();
> > >  
> > > +    struct MOZ_STACK_CLASS ReadSegmentsState {
> > > +        nsCOMPtr<nsIInputStream> mThisStream;
> > 
> > I'm a little skeptical of the goodness of the nsCOMPtr usage in this patch
> > (and other, similar patches); we're addref'ing |this|, which seems unlikely
> > to go away while we're reading from the stream.  (I realize the same sort of
> > argument is not as applicable to something like DOM code...)  What's your
> > rationale for adding strong references, rather than annotating?
>
> Mostly to bullet proof future code.  I think in cases where there are no
> performance concerns, we should prefer strong references.  It's hard to keep
> invariants valid otherwise.  WDYT?

Sure, but looking at |$VCS log|, this is pretty stable code, and we're unlikely to change it in radical ways anytime soon.

Sprinkling strong references about as a preemptive measure also means that you can easily find yourself in a situation where you're suddenly doing a bunch of completely unnecessary work.  If you really want to solve this properly, you should start making us use |const nsRefPtr<T>&| parameters. :)
Flags: needinfo?(nfroyd)
Comment on attachment 8550810 [details] [diff] [review]
Mark ReadSegmentsState as stack class, and its mThisStream as nsCOMPtr

Review of attachment 8550810 [details] [diff] [review]:
-----------------------------------------------------------------

I don't think this is really worth it, but it's also probably not worth arguing about.
Attachment #8550810 - Flags: review?(nfroyd) → review+
https://hg.mozilla.org/mozilla-central/rev/089b7c428261
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
You need to log in before you can comment on or make changes to this bug.