Eagerly drop nsStreamLoader::mData

RESOLVED FIXED in mozilla31

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: khuey, Assigned: bkelly)

Tracking

unspecified
mozilla31
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [MemShrink:P2][c= p=3 s= u=])

Attachments

(2 attachments, 1 obsolete attachment)

Is there a reason not to drop mData in nsStreamLoader right after firing the OnStreamComplete callback?  Waiting for the destructor means we're keeping potentially unbounded amounts of memory around unnecessarily.

See bug 993548 for an even more aggressive solution for scripts.
We should totally do this.
Assignee: nobody → bkelly
Status: NEW → ASSIGNED
I don't have a great workload to demonstrate memory savings, but I was able to browse facebook on device successfully with this applied.

https://tbpl.mozilla.org/?tree=Try&rev=47114d995d9b
Attachment #8403953 - Flags: review?(khuey)
Comment on attachment 8403953 [details] [diff] [review]
Eagerly free nsStreamLoader data. (v1)

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

I only know this code in a legacy sense, but this seems totally fine for me. mData can only be accessed from ODA which cannot be called after OnStopRequest. The class is MOZ_FINAL so it ought to be safe.
Attachment #8403953 - Flags: review?(khuey) → review+
Thanks Patrick!  Sorry I forgot to check modules owners list to get the right reviewer here.

Note, it looks like the try build is very unhappy.  I wonder if some sub-classes expect to find information in mData at later times.
There are no subclasses of nsStreamLoader.

Try is unhappy because you pushed on a broken base rev.  In fact, inbound is currently closed for that very same bustage...
there are no subclasses..

perhaps a recipient of onStreamComplete is not returning NS_SUCCESS_ADOPTED_DATA when it should be taking ownership of the data? (and continuing to reference it..)

(oh - I see bz has a more comforting theory :))
(In reply to Boris Zbarsky [:bz] from comment #5)
> There are no subclasses of nsStreamLoader.
> 
> Try is unhappy because you pushed on a broken base rev.  In fact, inbound is
> currently closed for that very same bustage...

That doesn't explain the xpcshell failures, does it?
It seem ASAN is also reporting a use-after-free.
kyle beat me to it in a comment race. +dkeeler
God.  PSM code strikes again.  Nothing like depending on things that are not documented to hold!

One other thing that is potentially wrong in the patch is that it changes the behavior on the numBytesRead property on streamloader.  It used to return the correct value unless OnStreamComplete returned NS_SUCCESS_ADOPTED_DATA.  Now it no longer does.  That said, that has only one consumer, and that consumer may not depend on the behavior after OnStreamComplete, so we could possibly change numBytesRead to only be meaningful if OnStreamComplete has not been called.
Seems like it should be straightforward to have that code return NS_SUCCESS_ADOPTED_DATA and then free it when it is done with the data.
fwiw nsistreamloader.idl does say that it frees the data in its destructor. should at least change the doc too.
I'm a bit short on time today, but will post updated patches tomorrow morning.
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #9)
> http://mxr.mozilla.org/mozilla-central/source/security/manager/ssl/src/
> nsNSSCallbacks.cpp#681
> 
> Lovely.

I'm hoping to replace/remove/improve a lot of that code. Some of the work may happen in bug 982248, but this improvement shouldn't be blocked on that. In the meantime, let me know if you need any help fixing what's wrong with PSM here.
Assignee: bkelly → mcmanus
a try run with both patches applied

https://tbpl.mozilla.org/?tree=Try&rev=715d4415e683

we should update that idl documentation to be clear.
Assignee: mcmanus → bkelly
Updated IDL documentation.  Carried r+ forward from previous review.
Attachment #8404860 - Flags: review+
Attachment #8403953 - Attachment is obsolete: true
For some reason the try build is still showing the bustage from bug 994216 even though it includes :myk's fix.  Looking at the hg parent for the patches, I think the push did not work as planned.  Let me update and push a new try.
Comment on attachment 8404386 [details] [diff] [review]
PSM HTTP Fetch should own streamloader data

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

Great - r=me.
Attachment #8404386 - Flags: review?(dkeeler) → review+
Try is looking good.  I was a bit worried about the orange in mac debug M1, but I finally got it to green.  Looks like lots of intermittent results on that one right now.

I'll land once the tree re-opens.
Attachment #8404386 - Flags: review?(honzab.moz)
(In reply to Ben Kelly [:bkelly] from comment #22)
> Try is looking good.  I was a bit worried about the orange in mac debug M1,
> but I finally got it to green.  Looks like lots of intermittent results on
> that one right now.
> 
> I'll land once the tree re-opens.

that psm patch requires a second review before landing. (see exceptions in http://www.mozilla.org/hacking/reviewers.html)
Thank you for catching me on this.  I'm sorry for not knowing that part of the process.  This is the first time I've hit a super review or second review situation.
Whiteboard: [MemShrink] → [MemShrink][c= p=3 s= u=]
Whiteboard: [MemShrink][c= p=3 s= u=] → [MemShrink:P2][c= p=3 s= u=]
Attachment #8404386 - Flags: review?(honzab.moz) → review+
https://hg.mozilla.org/mozilla-central/rev/800df0af4251
https://hg.mozilla.org/mozilla-central/rev/feded4c79308
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
You need to log in before you can comment on or make changes to this bug.