595972 - HttpChannelChild queueing can break - Abort: Queue flushing should not occur if PHASE_UNQUEUED

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect)

Description

[Josh Matthews [:jdm]]

cjones saw the "Queue flushing should not occur if PHASE_UNQUEUED" abort get triggered once.  My suspicion is that a non-listener callback calls channel.suspend(), which means that the channel suspends itself while in the unqueued state.

Comment 1

[Josh Matthews [:jdm]]

I hit this watching videos on michaelverdi.com.

Comment 2

[Josh Matthews [:jdm]]

Attachment: Backtrace

As I suspected, it looks like the video code calls Suspend and Resume willy-nilly.

Comment 3

[Josh Matthews [:jdm]]

Attachment: Culprit backtrace

The video thread posts a runnable to suspend the underlying channel.  Guh.  Relevant code is nsMediaCache::QueueUpdate().

Comment 5

[Josh Matthews [:jdm]]

Attachment: Fix event queue state invariants on HTTP channel resume.

It turns out to be a simple patch. The various tests at stake still pass, and watching videos doesn't crash.

Comment 6

[Jason Duell]

Comment on attachment 476504 [details] [diff] [review]
Fix event queue state invariants on HTTP channel resume.

>-  if (!mSuspendCount)
>+  if (!mSuspendCount) {
>+    // If we were suspended outside of an event handler (bug 595972) we'll
>+    // consider ourselves unqueued. This is a legal state of affairs but
>+    // FlushEventQueue() can't easily ensure this fact, so we'll do some
>+    // fudging to set the invariants correctly.    
>+    if (mQueuePhase == PHASE_UNQUEUED)
>+      mQueuePhase = PHASE_FINISHED_QUEUEING;
>     FlushEventQueue();
>+  }

OK, I just stared at the code here for a while, and I think this is correct, so +r to fix the bug.  But I'd like to keep the queuing code as simple and intuitive as it can be, and I suspect we've got a little more complexity than we need now.  In particular, as you move this code to make it usable by FTP as well, please ponder the following:

1) Do we still need ShouldEnqueue() to be 

    return mQueuePhase != PHASE_UNQUEUED || mSuspendCount

Or can we get rid of the mSuspendCount check (now that once we're suspended we won't ever be in PHASE_UNQUEUED) 

2)  Do we really need PHASE_FINISHED_QUEUEING, or can we merge it into PHASE_QUEUING?  From a quick (and quite possibly wrong) glance over the code, it looks like we always (well, before this patch) transition immediately from PHASE_FINISHED_QUEUEING to PHASE_FLUSHING.  The only check for PHASE_FINISHED_QUEUEING is in FlushEventQueue.  If we changed it there (and in suspend) to PHASE_FLUSHING, seems like everything would still work, and we'd have one less state in our machine.

Comment 7

[Josh Matthews [:jdm]]

1) Note that the patch changes Resume() not Suspend(), so mSuspendCount is still vital since we'll be in PHASE_UNQUEUED while suspended.  We don't want to be changing the state inside Suspend() because it could mess up other queueing logic if we happen to be flushing at that point.

2) Withtout PHASE_FINISHED_QUEUEING, I believe it would be much harder to determine whether we're in a valid state to be flushing the queue.  Yes, we always transition to PHASE_FLUSHING unless we're suspended, I think the logic is clearer when we can delineate that we're currently in a flushable state.

Comment 9

[Mark Finkle (:mfinkle) (use needinfo?)]

checkin-needed?

Comment 10

[Josh Matthews [:jdm]]

Yeah, I guess this is ready to be pushed. The tree's limited to b1 blockers right now, though.

Comment 11

[Michael Wu [:mwu]]

http://hg.mozilla.org/mozilla-central/rev/d6e2fc48375c