Closed
Bug 772528
Opened 12 years ago
Closed 8 years ago
nsPartialFileInputStream::Init() shouldn't call Seek()
Categories
(Core :: Networking: File, defect)
Core
Networking: File
Tracking
()
RESOLVED
FIXED
2.6 S6 - 1/29
Tracking | Status | |
---|---|---|
firefox48 | --- | fixed |
People
(Reporter: janv, Assigned: edenchuang)
References
Details
(Whiteboard: [necko-would-take])
Attachments
(1 file, 3 obsolete files)
The nsPartialFileInputStream class shouldn't call Seek() in the Init() method. It can cause main thread IO. It should rather do it in DoPendingOpen() See also bug 772167.
Assignee | ||
Comment 1•8 years ago
|
||
Hello Baku This bug is that I told you at Orlando, IO operation is executed on main thread. While constructing nsPartialFileInputStream, it will call nsFileInputStream::Seek() and try to open the file on main thread. This blocking the main thread, and FileSystemProvider API mechanism cannot dispatch OpenFileEvent to gaia, then whole B2G process is blocked. I wrote a patch to fix this issue. Instead of calling nsFileInputStream::Seek() in nsPartialFileInputStream::Init(), we call the Seek() when the first time the file stream is requested to perform some IO operations, for example read(). Could you take a look and give some feedback on it. Thanks.
Attachment #8701741 -
Flags: feedback?(amarchesini)
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → echuang
Comment 2•8 years ago
|
||
Comment on attachment 8701741 [details] [diff] [review] bug772528_v1.patch Review of attachment 8701741 [details] [diff] [review]: ----------------------------------------------------------------- ::: netwerk/base/nsFileStreams.cpp @@ +846,5 @@ > +nsPartialFileInputStream::DoPendingOpen() > +{ > + if (!mDeferredOpen) { > + return NS_OK; > + } Probably here you want t odo: mDeferredOpen = false; otherwise for each Read() you seek to start.
Attachment #8701741 -
Flags: feedback?(amarchesini) → feedback+
Assignee | ||
Comment 3•8 years ago
|
||
(In reply to Andrea Marchesini (:baku) from comment #2) > Comment on attachment 8701741 [details] [diff] [review] > bug772528_v1.patch > > Review of attachment 8701741 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: netwerk/base/nsFileStreams.cpp > @@ +846,5 @@ > > +nsPartialFileInputStream::DoPendingOpen() > > +{ > > + if (!mDeferredOpen) { > > + return NS_OK; > > + } > > Probably here you want t odo: > > mDeferredOpen = false; > > otherwise for each Read() you seek to start. No, I think we needn't set mDeferredOpen as false here. When nsFileInputStream::Seek() is called, the nsFileInputStream::DoPendingOpen() is also called. https://dxr.mozilla.org/mozilla-central/source/netwerk/base/nsFileStreams.cpp#527 nsFileInputStream::DoPendingOpen() opens the file and set mDeferredOpen as false. https://dxr.mozilla.org/mozilla-central/source/netwerk/base/nsFileStreams.cpp#370 That's the reason why I didn't cleanup mDeferredOpen in nsPartialFileInputStream::DoPendingOpen
Flags: needinfo?(amarchesini)
Assignee | ||
Updated•8 years ago
|
Attachment #8701741 -
Flags: review?(amarchesini)
Comment 5•8 years ago
|
||
Comment on attachment 8701741 [details] [diff] [review] bug772528_v1.patch Review of attachment 8701741 [details] [diff] [review]: ----------------------------------------------------------------- having if (NS_WARN_IF()) or NS_ENSURE_SUCCESS will add a NS_WARNING message. ::: netwerk/base/nsFileStreams.cpp @@ +716,1 @@ > if (NS_SUCCEEDED(rv)) { What about if we change this in: NS_ENSURE_SUCCESS(rv, rv); *aResult = tell - mStart; return NS_OK; @@ +726,5 @@ > + > + nsresult rv = DoPendingOpen(); > + NS_ENSURE_SUCCESS(rv, rv); > + > + rv = nsFileInputStream::Available(&available); same here. @@ +746,5 @@ > *aResult = 0; > return NS_OK; > } > > + rv = nsFileInputStream::Read(aBuf, readsize, aResult); and here. @@ +778,5 @@ > if (offset < (int64_t)mStart || offset > (int64_t)(mStart + mLength)) { > return NS_ERROR_INVALID_ARG; > } > > + rv = nsFileInputStream::Seek(NS_SEEK_SET, offset); and here.
Attachment #8701741 -
Flags: review?(amarchesini) → review+
Assignee | ||
Comment 6•8 years ago
|
||
[Approval Request Comment] Bug caused by (feature/regressing bug #): open() is executed at main thread. User impact if declined: No, there is no user impact. Testing completed: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d322356d4b7d Risk to taking this patch (and alternatives if risky): Low risky, it just postpones the open() call at IO thread while needed. String or UUID changes made by this patch: None
Attachment #8701741 -
Attachment is obsolete: true
Assignee | ||
Updated•8 years ago
|
Comment 8•8 years ago
|
||
backedout for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=20282465&repo=mozilla-inbound
Flags: needinfo?(echuang)
Updated•8 years ago
|
Whiteboard: [necko-would-take]
Assignee | ||
Comment 10•8 years ago
|
||
Force pending nsFileInputStream::Seek() while other operations are requested.
Attachment #8710894 -
Attachment is obsolete: true
Flags: needinfo?(echuang)
Attachment #8727235 -
Flags: review?(mcmanus)
Comment 11•8 years ago
|
||
Comment on attachment 8727235 [details] [diff] [review] Bug772528-Remove_nsFileInputSream::Seek()_from_nsPartialInpuatStream::Init().patch Review of attachment 8727235 [details] [diff] [review]: ----------------------------------------------------------------- I'm fine with :baku continuing the review
Attachment #8727235 -
Flags: review?(mcmanus) → review?(amarchesini)
Comment 12•8 years ago
|
||
Comment on attachment 8727235 [details] [diff] [review] Bug772528-Remove_nsFileInputSream::Seek()_from_nsPartialInpuatStream::Init().patch Review of attachment 8727235 [details] [diff] [review]: ----------------------------------------------------------------- ::: netwerk/base/nsFileStreams.cpp @@ +721,5 @@ > > NS_IMETHODIMP > nsPartialFileInputStream::Tell(int64_t *aResult) > { > + remove this extra line.
Attachment #8727235 -
Flags: review?(amarchesini) → review+
Assignee | ||
Comment 13•8 years ago
|
||
[Approval Request Comment] Bug caused by (feature/regressing bug #): IO operations are executed at main thread. User impact if declined: No, there is no user impact. Testing completed: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d4adb7d9b401 Risk to taking this patch (and alternatives if risky): Low risky. String or UUID changes made by this patch: None
Attachment #8727235 -
Attachment is obsolete: true
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Comment 14•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/422d5142ae42
Keywords: checkin-needed
Comment 15•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/422d5142ae42
You need to log in
before you can comment on or make changes to this bug.
Description
•