Last Comment Bug 852009 - Fail to do concurrent requests of GetCachedFileDescriptor()
: Fail to do concurrent requests of GetCachedFileDescriptor()
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: IPC (show other bugs)
: Trunk
: All Gonk (Firefox OS)
: -- normal (vote)
: mozilla24
Assigned To: John Lin [:jolin][:jhlin]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2013-03-17 19:58 PDT by Thinker Li [:sinker]
Modified: 2013-05-30 09:08 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch to address the assertion failure (4.28 KB, patch)
2013-04-29 03:05 PDT, John Lin [:jolin][:jhlin]
bent.mozilla: review+
Details | Diff | Review

Description Thinker Li [:sinker] 2013-03-17 19:58:36 PDT
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 C.J. Ku[:cjku] 2013-03-17 22:53:13 PDT
Assgin to John.
Comment 2 John Lin [:jolin][:jhlin] 2013-03-26 01:23:34 PDT
  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!) 2013-03-26 08:48:49 PDT
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 [:jolin][:jhlin] 2013-03-27 20:20:58 PDT
(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 [:jolin][:jhlin] 2013-04-01 05:03:45 PDT
(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 [:jolin][:jhlin] 2013-04-29 03:05:32 PDT
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.
Comment 7 Ben Turner (not reading bugmail, use the needinfo flag!) 2013-05-21 13:07:28 PDT
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 [:jolin][:jhlin] 2013-05-23 00:20:41 PDT
Tryserver run: https://tbpl.mozilla.org/?tree=Try&rev=7cce2a3b99bc
Comment 9 Ryan VanderMeulen [:RyanVM] 2013-05-29 17:59:11 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/4168e3babe79
Comment 10 Ryan VanderMeulen [:RyanVM] 2013-05-30 09:08:14 PDT
https://hg.mozilla.org/mozilla-central/rev/4168e3babe79

Note You need to log in before you can comment on or make changes to this bug.