Closed
Bug 1419382
Opened 7 years ago
Closed 7 years ago
NS_NewInputStreamPump, NS_NewInputStreamChannelInternal, ... should take ownership of the inputStream
Categories
(Core :: DOM: Core & HTML, enhancement)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla59
Tracking | Status | |
---|---|---|
firefox59 | --- | fixed |
People
(Reporter: baku, Assigned: baku)
Details
Attachments
(4 files, 3 obsolete files)
10.88 KB,
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
5.19 KB,
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
14.73 KB,
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
16.93 KB,
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Comment 1•7 years ago
|
||
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•7 years ago
|
Summary: NS_NewInputStreamPump should take ownership of the inputStream → NS_NewInputStreamPump, NS_NewInputStreamChannelInternal, ... should take ownership of the inputStream
Assignee | ||
Updated•7 years ago
|
Attachment #8930450 -
Attachment description: stream3.patch → part 3 - NS_NewInputStreamPump
Assignee | ||
Comment 2•7 years ago
|
||
Attachment #8930452 -
Flags: review?(bugs)
Assignee | ||
Comment 3•7 years ago
|
||
Attachment #8930454 -
Flags: review?(bugs)
Assignee | ||
Comment 4•7 years ago
|
||
Attachment #8930455 -
Flags: review?(bugs)
Comment 5•7 years 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•7 years ago
|
Attachment #8930454 -
Flags: review?(bugs) → review+
Comment 6•7 years 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•7 years ago
|
Attachment #8930455 -
Flags: review?(bugs) → review+
Assignee | ||
Comment 7•7 years ago
|
||
Attachment #8930452 -
Attachment is obsolete: true
Attachment #8930468 -
Flags: review?(bugs)
Assignee | ||
Comment 8•7 years 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•7 years ago
|
||
Attachment #8930450 -
Attachment is obsolete: true
Attachment #8930470 -
Flags: review?(bugs)
Updated•7 years ago
|
Attachment #8930468 -
Flags: review?(bugs) → review+
Comment 10•7 years 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•7 years ago
|
||
Attachment #8930470 -
Attachment is obsolete: true
Attachment #8930547 -
Flags: review?(bugs)
Comment 12•7 years 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•7 years 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•7 years 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
Closed: 7 years ago
status-firefox59:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•