Closed Bug 772528 Opened 8 years ago Closed 4 years ago

nsPartialFileInputStream::Init() shouldn't call Seek()

Categories

(Core :: Networking: File, defect)

defect
Not set

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.
Attached patch bug772528_v1.patch (obsolete) — Splinter Review
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)
Blocks: 1187223
Assignee: nobody → echuang
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+
(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)
I see.
Flags: needinfo?(amarchesini)
Attachment #8701741 - Flags: review?(amarchesini)
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+
[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
Status: NEW → ASSIGNED
Keywords: checkin-needed
Target Milestone: --- → 2.6 S6 - 1/29
Whiteboard: [necko-would-take]
Force pending nsFileInputStream::Seek() while other operations are requested.
Attachment #8710894 - Attachment is obsolete: true
Flags: needinfo?(echuang)
Attachment #8727235 - Flags: review?(mcmanus)
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 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+
[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
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/422d5142ae42
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.