Closed Bug 865337 Opened 9 years ago Closed 9 years ago

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


(Firefox OS Graveyard :: General, defect)

Gonk (Firefox OS)
Not set


(Not tracked)



(Reporter: kats, Assigned: kats)




(1 file, 1 obsolete file)

Attached patch Patch (obsolete) — Splinter Review
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.
Attachment #741400 - Flags: review?(bent.mozilla)
Comment on attachment 741400 [details] [diff] [review]

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?
Attachment #741400 - Flags: review?(bent.mozilla)
Attached patch Patch v2Splinter Review
Yup, that makes sense. Updated patch attached.
Attachment #741400 - Attachment is obsolete: true
Attachment #742189 - Flags: review?(bent.mozilla)
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!"?
Attachment #742189 - Flags: review?(bent.mozilla) → review+
It looks like this code uses 4-space indentation. Silly as that as, please don't introduce mixed 2-space/4-space indentation.
Assignee: nobody → bugmail.mozilla
(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.
Closed: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.