865337 - Assertion failure at TabChild.cpp:1293: MOZ_ASSERT(!info->mCallback)

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[Kartikaya Gupta (email:[e-mail])]

Attachment: Patch

I was seeing this assertion failure yesterday when I built a debug/no-opt B2G build for unagi from BRANCH=master. It was blocking me completely since it got into a crash loop while I was trying to start the browser on the device. I attached gdb and discovered that info->mCallback was non-null and info->mCanceled was true. I don't know this code at all but looking at it I believe there is an unhandled case where GetCachedFileDescriptor is called, then the call is canceled by CancelCachedFileDescriptorCallback, and then GetCachedFielDescriptor is called again, all before RecvCacheFileDescriptor is called. I used the attach patch to work around the problem but I don't know if it's "correct" or not.

Comment 1

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

Comment on attachment 741400 [details] [diff] [review]
Patch

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

Every call to GetCachedFileDescriptor should result in one later call to RecvCacheFileDescriptor, so if GetCachedFileDescriptor was called twice then there should be two calls later to RecvCacheFileDescriptor. With your patch it looks like the second RecvCacheFileDescriptor would not know that it is unneeded and would therefore stick around forever.

I think the better fix here would be to insert a new CachedFileDescriptorInfo with the second callback into the array *before* the canceled one. That way the first fd would run with the second callback and then the second fd would be immediately closed.

Does that make sense?

Comment 2

[Kartikaya Gupta (email:[e-mail])]

Attachment: Patch v2

Yup, that makes sense. Updated patch attached.

Comment 3

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

Comment on attachment 742189 [details] [diff] [review]
Patch v2

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

Thanks for digging in here! r=me with this one comment.

::: dom/ipc/TabChild.cpp
@@ +1295,5 @@
> +    // canceled, insert the new request ahead of the old in the queue so that
> +    // it will be serviced first.
> +    if (info->mCanceled) {
> +      mCachedFileDescriptorInfos.InsertElementAt(index,
> +          new CachedFileDescriptorInfo(aPath, aCallback));

This is slightly scary because it will invalidate |info|, but I can't see how it would be really dangerous as long as we don't touch |info| again... Maybe just add a comment to that effect immediately before the InsertElementAt call? Something like "This insertion will change the array and invalidate |info|, don't touch |info| after this!"?

Comment 4

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

It looks like this code uses 4-space indentation. Silly as that as, please don't introduce mixed 2-space/4-space indentation.

Comment 5

[Kartikaya Gupta (email:[e-mail])]

(In reply to :Ms2ger from comment #4)
> It looks like this code uses 4-space indentation. Silly as that as, please
> don't introduce mixed 2-space/4-space indentation.

Sort of. The file already has mixed indentation, and the vim modeline at the top sets it to 2 spaces which is why my code turned out that way. But yeah, for consistency within this function at least I'll change it to 4 spaces.

Comment 6

[Kartikaya Gupta (email:[e-mail])]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ae2ec3465251

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ae2ec3465251