Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Fail to do concurrent requests of GetCachedFileDescriptor()

RESOLVED FIXED in mozilla24

Status

()

Core
IPC
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: sinker, Assigned: jolin)

Tracking

Trunk
mozilla24
All
Gonk (Firefox OS)
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

4.28 KB, patch
Ben Turner (not reading bugmail, use the needinfo flag!)
: review+
Details | Diff | Splinter Review
(Reporter)

Description

4 years ago
The child process will be wrong by starting a GetCachedFileDescriptor() before the finish of previous call.  It is failed by an assertion of CachedFileDescriptorInfo::mCallback.  The right behavior is adding a new CachedFileDescriptorInfo if the passed callback does not match any mCallback of existing CacheFileDescriptorInfo, and fire all callbacks once the FileDescriptor had been received from the parent.
(Reporter)

Updated

4 years ago
Summary: Fail to do current requests of GetCachedFileDescriptor() → Fail to do concurrent requests of GetCachedFileDescriptor()
Assgin to John.
Assignee: nobody → jolin
(Assignee)

Comment 2

4 years ago
  The code in question was introduced in bug 835698.
  AFAICT, after going through the long list of comments there, a similar problem (failing MOZ_ASSERT(!info->mCallback)) happened back then too (bug 835698,comment 16) and :bent addressed it by modifying nsJARChannel to avoid multi-request from child (attachment 709768 [details] [diff] [review]).
  It seems to me that we can either use the same trick in the code where :sinker reproduced the issue (and add some comment to RemoteOpenFileChild::AsyncRemoteFileOpen() to document its limitation), or moving the fix from nsJARChannel to RemoteOpenFileChild. Since I don't see any downside in the 2nd way, I'm going to start working on that (which might take a while since I'm still digesting the huge code base called mozilla).
  Any comments are welcome.
Can you give me a little more information about how you're reproducing this assertion failure? Are you seeing it in the current code or only with some additional changes applied?

I don't see any reason why you would want to ask the parent for multiple fds for the same file. We should just wait on the first request to finish and then dup() as needed, right?
(Assignee)

Comment 4

4 years ago
(In reply to ben turner [:bent] from comment #3)
> Can you give me a little more information about how you're reproducing this
> assertion failure? Are you seeing it in the current code or only with some
> additional changes applied?
It seems Thinker had exactly the same case as bug 835698,comment 16. Thinker, could you please provide more info?

> I don't see any reason why you would want to ask the parent for multiple fds
> for the same file. We should just wait on the first request to finish and
> then dup() as needed, right?
Of course.
What I thought was that if the problem happened at least twice (before I realized it's the same case :)), it would be nice for RemoteOpenFileChild::AsyncRemoteFileOpen() to provide more protection (other than the one your assertion does in debug build).
Flags: needinfo?(tlee)
(Assignee)

Comment 5

4 years ago
(In reply to John Lin[:jolin][:jhlin] from comment #4)
> (In reply to ben turner [:bent] from comment #3)
> > Can you give me a little more information about how you're reproducing this
> > assertion failure? Are you seeing it in the current code or only with some
> > additional changes applied?
> It seems Thinker had exactly the same case as bug 835698,comment 16.
> Thinker, could you please provide more info?
> 

Not sure if Thinker had the same situation but I managed to reproduce the assertion failure using some BT private branch from Shawn.
In short, it goes like this:
(1) [Child] RemoteOpenFileChild::AsyncRemoteFileOpen()
    => [Child] TabChild::GetCachedFileDescriptor() // append CachedFileDescriptorInfo
(2) [Parent] RemoteOpenFileParent::OpenFileAndSendFDRunnable::SendResponse()
    => [Parent] TabParent::SendCacheFileDescriptor()
    => [Child] TabChild::RecvCachedFileDescritor() // remove CachedFileDescriptorInfo
(3) [Child] memory pressure => nsZipReaderCache::Observe() // flush cache
(4) [Child] RemoteOpenFileChild::AsyncRemoteFileOpen()
    => TabChild::GetCachedFileDescriptor() // append CachedFileDescriptorInfo
(5) [Child] memory pressure => nsZipReaderCache::Observe() // flush cache
(6) [Child] RemoteOpenFileChild::AsyncRemoteFileOpen()
    => TabChild::GetCachedFileDescriptor() // CachedFileDescriptorInfo from (4) is still there. Assertion failure at TabChild.cpp:1293(and 1294)!
Flags: needinfo?(tlee)
(Assignee)

Comment 6

4 years ago
Created attachment 742983 [details] [diff] [review]
Patch to address the assertion failure

A flag was added for the child to check whether it should expect cached FD (which should be sent only once for app package ZIP) from parent.
Attachment #742983 - Flags: review?(bent.mozilla)
Comment on attachment 742983 [details] [diff] [review]
Patch to address the assertion failure

Review of attachment 742983 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good to me!
Attachment #742983 - Flags: review?(bent.mozilla) → review+
(Assignee)

Comment 8

4 years ago
Tryserver run: https://tbpl.mozilla.org/?tree=Try&rev=7cce2a3b99bc
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/4168e3babe79
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/4168e3babe79
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
You need to log in before you can comment on or make changes to this bug.