1419382 - NS_NewInputStreamPump, NS_NewInputStreamChannelInternal, ... should take ownership of the inputStream

Status: RESOLVED FIXED

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

Comment 1

[Andrea Marchesini [:baku]]

Attachment: part 3 - NS_NewInputStreamPump

I have several patches about changing the ownership model for nsIInputStream functions. This is one of them.

Comment 2

[Andrea Marchesini [:baku]]

Attachment: part 1 - NS_NewInputStreamChannelInternal

Comment 3

[Andrea Marchesini [:baku]]

Attachment: part 2 - NS_NewInputStreamChannel

Comment 4

[Andrea Marchesini [:baku]]

Attachment: part 4 - Get rid of NS_NewAsyncStreamCopier

Comment 5

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

Comment on attachment 8930452 [details] [diff] [review]
part 1 - NS_NewInputStreamChannelInternal


>   rv = NS_NewInputStreamChannelInternal(aResult,
>                                         aURI,
>-                                        stream,
>+                                        stream.forget(),
>                                         NS_LITERAL_CSTRING(UNKNOWN_CONTENT_TYPE),
>                                         EmptyCString(), // aContentCharset
>                                         aLoadInfo);
>   if (NS_SUCCEEDED(rv)) {
>     stream->SetChannel(*aResult);
This crashes since stream is now null

Comment 6

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

Comment on attachment 8930450 [details] [diff] [review]
part 3 - NS_NewInputStreamPump

># HG changeset patch
># User Andrea Marchesini <[e-mail]>
># Parent  10701fa9c42bc9b85db1bc17f376f312bac0eb53
>
>diff --git a/dom/fetch/FetchConsumer.cpp b/dom/fetch/FetchConsumer.cpp
>--- a/dom/fetch/FetchConsumer.cpp
>+++ b/dom/fetch/FetchConsumer.cpp
>@@ -467,17 +467,17 @@ FetchBodyConsumer<Derived>::BeginConsume
>   if (mShuttingDown) {
>     // We haven't started yet, but we have been terminated. AutoFailConsumeBody
>     // will dispatch a runnable to release resources.
>     return;
>   }
> 
>   nsCOMPtr<nsIInputStreamPump> pump;
>   nsresult rv = NS_NewInputStreamPump(getter_AddRefs(pump),
>-                                      mBodyStream, 0, 0, false,
>+                                      mBodyStream.forget(), 0, 0, false,
I have no idea why this is right. Clearing a member variable this way looks scary. 
Please explain and at least add a good comment why this is ok, and ask review again.
>+++ b/modules/libjar/zipwriter/nsZipWriter.cpp
>@@ -983,17 +983,18 @@ inline nsresult nsZipWriter::BeginProces
> 
>         RefPtr<nsZipDataStream> stream = new nsZipDataStream();
>         NS_ENSURE_TRUE(stream, NS_ERROR_OUT_OF_MEMORY);
>         rv = stream->Init(this, mStream, header, aItem->mCompression);
>         NS_ENSURE_SUCCESS(rv, rv);
> 
>         if (aItem->mStream) {
>             nsCOMPtr<nsIInputStreamPump> pump;
>-            rv = NS_NewInputStreamPump(getter_AddRefs(pump), aItem->mStream,
>+            rv = NS_NewInputStreamPump(getter_AddRefs(pump),
>+                                       aItem->mStream.forget(),
This looks scary too. Why is this ok. Needs a good comment. Why is clearing a member variable ok?


> inline nsresult nsZipWriter::BeginProcessingRemoval(int32_t aPos)
> {
>     // Open the zip file for reading
>     nsCOMPtr<nsIInputStream> inputStream;
>     nsresult rv = NS_NewLocalFileInputStream(getter_AddRefs(inputStream),
>                                              mFile);
>     NS_ENSURE_SUCCESS(rv, rv);
>     nsCOMPtr<nsIInputStreamPump> pump;
>-    rv = NS_NewInputStreamPump(getter_AddRefs(pump), inputStream, 0, 0, true);
>+    rv = NS_NewInputStreamPump(getter_AddRefs(pump), inputStream.forget(), 0, 0,
>+                               true);
>     if (NS_FAILED(rv)) {
>         inputStream->Close();
You crash here if you do inputStream.forget() above


>         return rv;
>     }
>     nsCOMPtr<nsIStreamListener> listener;
>     rv = NS_NewSimpleStreamListener(getter_AddRefs(listener), mStream, this);
>     if (NS_FAILED(rv)) {
>         inputStream->Close();
You crash here if you do inputStream.forget() above

Comment 7

[Andrea Marchesini [:baku]]

Attachment: part 1 - NS_NewInputStreamChannelInternal

Comment 8

[Andrea Marchesini [:baku]]

> >+++ b/modules/libjar/zipwriter/nsZipWriter.cpp
> >@@ -983,17 +983,18 @@ inline nsresult nsZipWriter::BeginProces
> > 
> >         RefPtr<nsZipDataStream> stream = new nsZipDataStream();
> >         NS_ENSURE_TRUE(stream, NS_ERROR_OUT_OF_MEMORY);
> >         rv = stream->Init(this, mStream, header, aItem->mCompression);
> >         NS_ENSURE_SUCCESS(rv, rv);
> > 
> >         if (aItem->mStream) {
> >             nsCOMPtr<nsIInputStreamPump> pump;
> >-            rv = NS_NewInputStreamPump(getter_AddRefs(pump), aItem->mStream,
> >+            rv = NS_NewInputStreamPump(getter_AddRefs(pump),
> >+                                       aItem->mStream.forget(),
> This looks scary too. Why is this ok. Needs a good comment. Why is clearing
> a member variable ok?

I check how aItem is used and mFile, mStream and so on are used only at BeginProcessingAddition(). But probably it's better to keep the member variable as it is.

Comment 9

[Andrea Marchesini [:baku]]

Attachment: part 3 - NS_NewInputStreamPump

Comment 10

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

Comment on attachment 8930470 [details] [diff] [review]
part 3 - NS_NewInputStreamPump

This is same as part 4.

Comment 11

[Andrea Marchesini [:baku]]

Attachment: part 3 - NS_NewInputStreamPump

Comment 12

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

Comment on attachment 8930547 [details] [diff] [review]
part 3 - NS_NewInputStreamPump

ok, so mBodyStream.forget() is ok because mConsumeBodyPump sort of wraps that.


I'm not 100% sure this all in this bug is very useful. Maybe it is.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/04d75d37c343
Moving ownership of nsIInputStream when using netUtil functions - part 1 - NS_NewInputStreamChannelInternal, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/6b49a0395cb4
Moving ownership of nsIInputStream when using netUtil functions - part 2 - NS_NewInputStreamChannel, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/53f7c44ea855
Moving ownership of nsIInputStream when using netUtil functions - part 3 - NS_NewInputStreamPump, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/6abf85475411
Moving ownership of nsIInputStream when using netUtil functions - part 4 - Get rid of NS_NewAsyncStreamCopier, r=smaug

Comment 14

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/04d75d37c343
https://hg.mozilla.org/mozilla-central/rev/6b49a0395cb4
https://hg.mozilla.org/mozilla-central/rev/53f7c44ea855
https://hg.mozilla.org/mozilla-central/rev/6abf85475411