852009 - Fail to do concurrent requests of GetCachedFileDescriptor()

Status: RESOLVED FIXED

(Core :: IPC, defect)

Description

[Thinker Li [:sinker]]

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.

Comment 1

[u459114]

Assgin to John.

Comment 2

[John Lin [:jhlin][:jolin]]

  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.

Comment 3

[Ben Turner (not reading bugmail, use the needinfo flag!)]

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?

Comment 4

[John Lin [:jhlin][:jolin]]

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

Comment 5

[John Lin [:jhlin][:jolin]]

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

Comment 6

[John Lin [:jhlin][:jolin]]

Attachment: 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.

Comment 7

[Ben Turner (not reading bugmail, use the needinfo flag!)]

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!

Comment 8

[John Lin [:jhlin][:jolin]]

Tryserver run: https://tbpl.mozilla.org/?tree=Try&rev=7cce2a3b99bc

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4168e3babe79

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4168e3babe79