993591 - Eagerly drop nsStreamLoader::mData

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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.

Comment 1

[Boris Zbarsky [:bzbarsky]]

We should totally do this.

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Eagerly free nsStreamLoader data. (v1)

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

Comment 3

[Patrick McManus [:mcmanus]]

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.

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 5

[Boris Zbarsky [:bzbarsky]]

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...

Comment 6

[Patrick McManus [:mcmanus]]

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 :))

Comment 7

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(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?

Comment 8

[Ben Kelly [:bkelly, not reviewing]]

It seem ASAN is also reporting a use-after-free.

Comment 9

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

http://mxr.mozilla.org/mozilla-central/source/security/manager/ssl/src/nsNSSCallbacks.cpp#681

Lovely.

Comment 10

[Patrick McManus [:mcmanus]]

kyle beat me to it in a comment race. +dkeeler

Comment 11

[Boris Zbarsky [:bzbarsky]]

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.

Comment 12

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 13

[Patrick McManus [:mcmanus]]

fwiw nsistreamloader.idl does say that it frees the data in its destructor. should at least change the doc too.

Comment 14

[Ben Kelly [:bkelly, not reviewing]]

I'm a bit short on time today, but will post updated patches tomorrow morning.

Comment 15

[Dana Keeler (she/her) (use needinfo) (:keeler for reviews)]

(In reply to Kyle Huey [:khuey] ([e-mail]) 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.

Comment 16

[Patrick McManus [:mcmanus]]

Attachment: PSM HTTP Fetch should own streamloader data

Comment 17

[Patrick McManus [: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.

Comment 18

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Eagerly free nsStreamLoader data. (v2)

Updated IDL documentation.  Carried r+ forward from previous review.

Comment 19

[Ben Kelly [:bkelly, not reviewing]]

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 20

[Ben Kelly [:bkelly, not reviewing]]

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

Comment 21

[Dana Keeler (she/her) (use needinfo) (:keeler for reviews)]

Comment on attachment 8404386 [details] [diff] [review]
PSM HTTP Fetch should own streamloader data

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

Great - r=me.

Comment 22

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 23

[Patrick McManus [:mcmanus]]

(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)

Comment 24

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 25

[Ben Kelly [:bkelly, not reviewing]]

Thanks Honza!

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/800df0af4251
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/feded4c79308

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/800df0af4251
https://hg.mozilla.org/mozilla-central/rev/feded4c79308