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

RESOLVED FIXED in Firefox 59

Status

()

Core
DOM
RESOLVED FIXED
2 months ago
2 months ago

People

(Reporter: baku, Assigned: baku)

Tracking

unspecified
mozilla59
Points:
---

Firefox Tracking Flags

(firefox59 fixed)

Details

Attachments

(4 attachments, 3 obsolete attachments)

Comment hidden (empty)
(Assignee)

Comment 1

2 months ago
Created attachment 8930450 [details] [diff] [review]
part 3 - NS_NewInputStreamPump

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

Updated

2 months ago
Summary: NS_NewInputStreamPump should take ownership of the inputStream → NS_NewInputStreamPump, NS_NewInputStreamChannelInternal, ... should take ownership of the inputStream
(Assignee)

Updated

2 months ago
Attachment #8930450 - Attachment description: stream3.patch → part 3 - NS_NewInputStreamPump
(Assignee)

Comment 2

2 months ago
Created attachment 8930452 [details] [diff] [review]
part 1 - NS_NewInputStreamChannelInternal
Attachment #8930452 - Flags: review?(bugs)
(Assignee)

Comment 3

2 months ago
Created attachment 8930454 [details] [diff] [review]
part 2 - NS_NewInputStreamChannel
Attachment #8930454 - Flags: review?(bugs)
(Assignee)

Comment 4

2 months ago
Created attachment 8930455 [details] [diff] [review]
part 4 - Get rid of NS_NewAsyncStreamCopier
Attachment #8930455 - Flags: review?(bugs)

Comment 5

2 months ago
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-

Updated

2 months ago
Attachment #8930454 - Flags: review?(bugs) → review+

Comment 6

2 months ago
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-

Updated

2 months ago
Attachment #8930455 - Flags: review?(bugs) → review+
(Assignee)

Comment 7

2 months ago
Created attachment 8930468 [details] [diff] [review]
part 1 - NS_NewInputStreamChannelInternal
Attachment #8930452 - Attachment is obsolete: true
Attachment #8930468 - Flags: review?(bugs)
(Assignee)

Comment 8

2 months ago
> >+++ 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.
(Assignee)

Comment 9

2 months ago
Created attachment 8930470 [details] [diff] [review]
part 3 - NS_NewInputStreamPump
Attachment #8930450 - Attachment is obsolete: true
Attachment #8930470 - Flags: review?(bugs)

Updated

2 months ago
Attachment #8930468 - Flags: review?(bugs) → review+

Comment 10

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

This is same as part 4.
Attachment #8930470 - Flags: review?(bugs)
(Assignee)

Comment 11

2 months ago
Created attachment 8930547 [details] [diff] [review]
part 3 - NS_NewInputStreamPump
Attachment #8930470 - Attachment is obsolete: true
Attachment #8930547 - Flags: review?(bugs)

Comment 12

2 months ago
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+

Comment 13

2 months ago
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

Comment 14

2 months ago
bugherder
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
Status: NEW → RESOLVED
Last Resolved: 2 months ago
status-firefox59: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
You need to log in before you can comment on or make changes to this bug.