1409327 - NS_NewBufferedInputStream should take the ownership of the inputStream

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, enhancement, P2)

Comment 1

[Andrea Marchesini [:baku]]

Attachment: fix2.patch

As other wrapper, it's important that the bufferedInputStream takes the ownership of the input stream in order to avoid the use of it outside the wrapper.

Comment 2

[Andrea Marchesini [:baku]]

Attachment: fix2.patch

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8919242 [details] [diff] [review]
fix2.patch

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

::: netwerk/base/nsNetUtil.cpp
@@ +1370,5 @@
>  }
>  
>  MOZ_MUST_USE nsresult
> +NS_NewBufferedInputStream(nsIInputStream** aResult,
> +                          already_AddRefed<nsIInputStream> aInputStream,

Doesn't this mean if NS_NewBufferedInputStream() returns failure you lose the ref?  How about use nsCOMPtr<nsIInputStream>& and Move() it if you successfully take ownership.

Also, this is a pretty weak protection anyway.  Code can easily make nsCOMPtr<> on the stack to pass to this, etc.

Comment 4

[Andrea Marchesini [:baku]]

> Doesn't this mean if NS_NewBufferedInputStream() returns failure you lose
> the ref?  How about use nsCOMPtr<nsIInputStream>& and Move() it if you
> successfully take ownership.

This fails only if we have allocation issue. And usually we are talking about 512, 4096 bytes. We can probably mark this method as infallible.

> Also, this is a pretty weak protection anyway.  Code can easily make
> nsCOMPtr<> on the stack to pass to this, etc.

Currently we don't have any better mechanism to protect bugs like what we had in FileReader where an inputStream was used via BufferedInputStream and outside it. This is an extra operation to do, and a reviewer can maybe catch this as a bug.

Comment 5

[Andrea Marchesini [:baku]]

> This fails only if we have allocation issue. And usually we are talking
> about 512, 4096 bytes. We can probably mark this method as infallible.

Actually, 'new' doesn't fail: https://dxr.mozilla.org/mozilla-central/source/netwerk/base/nsBufferedStreams.cpp#67-68,77

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8919242 [details] [diff] [review]
fix2.patch

>@@ -469,47 +469,48 @@ nsSAXXMLReader::ParseFromString(const ns
>   nsresult rv = NS_NewByteInputStream(getter_AddRefs(stream),
>                                       data.get(), data.Length(),
>                                       NS_ASSIGNMENT_DEPEND);
>   NS_ENSURE_SUCCESS(rv, rv);
>   return ParseFromStream(stream, "UTF-8", aContentType);
> }
> 
> NS_IMETHODIMP
>-nsSAXXMLReader::ParseFromStream(nsIInputStream *aStream,
>+nsSAXXMLReader::ParseFromStream(nsIInputStream *aStreamPtr,
I don't understand this renaming. Ptr postfix is unusual.
I assume you did the change in order to guarantee other fixes are right, but could you undo this renaming

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4666cde20909
NS_NewBufferedInputStream should take the ownership of the inputStream, r=smaug

Comment 8

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out bug 1409394, bug 1409329 and bug 1409327 for failing browser-chrome's browser_save_link-perwindowpb.js and browser_libraryDrop.js on OS X and leaks detected by chrome test on Linux x64 asan:

Bug 1409394
https://hg.mozilla.org/integration/mozilla-inbound/rev/a461a81095f8b734427f244e1bb7e09d53713e65

Bug 1409329
https://hg.mozilla.org/integration/mozilla-inbound/rev/ec2f3cbc85809e0d9937383f1a0db0750dd9ed1f

Bug 1409327
https://hg.mozilla.org/integration/mozilla-inbound/rev/cb8c65940190581eb878f9999ae71a50d9fec873

Push which ran many failing tests: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=8b0c016826e027fad395b6e6e11d8fe178080489&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception
Likely fallout from bug 1409329.

Comment 9

[Ben Kelly [:bkelly, not reviewing]]

Is there some stream that needs to have Close() called on it that isn't getting it any more since we're now clearing the reference in these patches?

Comment 10

[Ben Kelly [:bkelly, not reviewing]]

And I'm sorry, but I pushed bug 1204254 which will conflict with your patches a bit.  (I removed one of the uses of this API.)

Comment 11

[Andrea Marchesini [:baku]]

(In reply to Ben Kelly [:bkelly] from comment #9)
> Is there some stream that needs to have Close() called on it that isn't
> getting it any more since we're now clearing the reference in these patches?

If we do that, then this is a bug. Closing a BufferedInputStream means closing the original stream as well.
If we close the originalStream without closing the bufferedInputStream, this is definitely a bug.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/23e2e65211f5
NS_NewBufferedInputStream should take the ownership of the inputStream, r=smaug

Comment 13

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/23e2e65211f5