Closed Bug 1419382 Opened 3 years ago Closed 3 years ago

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

Categories

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

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox59 --- fixed

People

(Reporter: baku, Assigned: baku)

Details

Attachments

(4 files, 3 obsolete files)

No description provided.
Attached patch part 3 - NS_NewInputStreamPump (obsolete) — Splinter Review
I have several patches about changing the ownership model for nsIInputStream functions. This is one of them.
Assignee: nobody → amarchesini
Attachment #8930450 - Flags: review?(bugs)
Summary: NS_NewInputStreamPump should take ownership of the inputStream → NS_NewInputStreamPump, NS_NewInputStreamChannelInternal, ... should take ownership of the inputStream
Attachment #8930450 - Attachment description: stream3.patch → part 3 - NS_NewInputStreamPump
Attachment #8930452 - Flags: review?(bugs)
Attachment #8930454 - Flags: review?(bugs)
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
Attachment #8930452 - Flags: review?(bugs) → review-
Attachment #8930454 - Flags: review?(bugs) → review+
Comment on attachment 8930450 [details] [diff] [review]
part 3 - NS_NewInputStreamPump

># HG changeset patch
># User Andrea Marchesini <amarchesini@mozilla.com>
># 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
Attachment #8930450 - Flags: review?(bugs) → review-
Attachment #8930455 - Flags: review?(bugs) → review+
Attachment #8930452 - Attachment is obsolete: true
Attachment #8930468 - Flags: review?(bugs)
> >+++ 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.
Attached patch part 3 - NS_NewInputStreamPump (obsolete) — Splinter Review
Attachment #8930450 - Attachment is obsolete: true
Attachment #8930470 - Flags: review?(bugs)
Attachment #8930468 - Flags: review?(bugs) → review+
Comment on attachment 8930470 [details] [diff] [review]
part 3 - NS_NewInputStreamPump

This is same as part 4.
Attachment #8930470 - Flags: review?(bugs)
Attachment #8930470 - Attachment is obsolete: true
Attachment #8930547 - Flags: review?(bugs)
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.
Attachment #8930547 - Flags: review?(bugs) → review+
Pushed by amarchesini@mozilla.com:
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
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.