584604 - Clean up HTTP buffering of incoming IPDL necko messages

Status: RESOLVED FIXED

(Core :: IPC, defect)

Description

[Doug Turner (:dougt)]

Follow up to 559200:

1) Fix up somewhat nonobvious somewhat invariant that chris pointed out in comment 55.

2) Reduce memory traffic by changing methods to only allocate and AppendToBuffer when we need to (also see comment 55)

Comment 1

[Josh Matthews [:jdm]]

From cjones:

>Also, it's possible to do sync XHR in xpcshell, right?  Would be nice to have a test for this.

From jduell:

>1) Some OnStatus (and I believe OnProgress) messages arrive and should be dispatched *before* OnStartRequest.   This patch looks like it breaks that.  To
know for sure (and to add test coverage), please modify test_progress.js so
that it also handles OnStart/Data/Stop.  Have all of them (inc
onStatus/Progress) print when they're called.   Run in unit, and then make a
wrapper in unit_ipc and run.  See if the order of calls differ.  If it does,
change the test so that it keeps a state variable to enforce that e10s calls
all the notifications/callbacks in the same order as non-e10s (I'm guessing
we'll want to guarantee that OnStatus and/or OnProgress have been called before
OnStartReq.  If we want to get fancy, OnStatus and OnProgress should also be
called again afterward, before each OnDataAvail.)

2) Why are we only buffering until OnStartRequest completes?  I'm guessing it's
quite possible for a sync XHR request to happen within OnDataAvailable (if HTML
parser hits JS that does one before the page has finished loading?  Not my
department--anyone know?).  Or an OnProgress/Status, or even OnStop.  There's
nothing in the API that forbids that. 

I was figuring we'd have a mechanism that buffered incoming msgs if *any* other
message is still being dispatched (or if the channel's suspended).  I.e. just
guarantee that all necko IPDL messages are dispatched serially.  

nit: rename "Callback" something more descriptive and less likely to cause
namespace and/or mxr clash?  How about "ChannelChildEvent"?

Comment 2

[Josh Matthews [:jdm]]

Attachment: Patch

All comments addressed.

Comment 3

[Josh Matthews [:jdm]]

Attachment: Progress test changes

Progress test changes.

Comment 4

[Josh Matthews [:jdm]]

Attachment: Sync XHR test

Now with more sync XHR testing.

Comment 5

[Josh Matthews [:jdm]]

Attachment: Progress test changes

Same as the previous progress test, but now with fewer leakage reports from xpcshell.

Comment 6

[Josh Matthews [:jdm]]

Attachment: Patch

I did some more thinking and added some more integrity checks.  The OnStartRequest listener failing will also no longer keep us buffering until the universe undergoes heat death.

Comment 7

[Josh Matthews [:jdm]]

Attachment: Sync XHR test

Updated xhr test.  It turns out the previous version was completely bogus, in all possible senses of the word.

Comment 8

[Josh Matthews [:jdm]]

Attachment: Progress test changes

The previous progress test changes actually broke all testing.  Woo.

Comment 9

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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

Disclaimer: I scanned this patch, but didn't do a detailed review since I don't know what's required re: necko.

>diff --git a/netwerk/protocol/http/HttpChannelChild.h b/netwerk/protocol/http/HttpChannelChild.h
>--- a/netwerk/protocol/http/HttpChannelChild.h
>+++ b/netwerk/protocol/http/HttpChannelChild.h
>   // Workaround for Necko re-entrancy dangers. We buffer all messages
>   // received until OnStartRequest completes.
>-  nsTArray<nsAutoPtr<Callback> > mBufferedCallbacks;
>-  bool mShouldBuffer;
>+  nsTArray<nsAutoPtr<ChildChannelEvent> > mBufferedCallbacks;
>+  enum {
>+    PHASE_UNBUFFERED,
>+    PHASE_BUFFERED,
>+    PHASE_FLUSHING_BUFFER
>+  } mBufferPhase;
> 
>-  bool BufferOrDispatch(Callback* callback);
>+  void BeginIPDLBuffering();
>+  bool FlushBuffer();
>+  bool BufferEvent(ChildChannelEvent* callback);
>+  bool ShouldBuffer();
>+  void EnsureUnbuffered();
> 

These changes look OK to me.  Some nits: where used as a verb, I would s/buffer/enqueue/ to make semantics clearer.  Second, BeginIPDLBuffering() isn't particularly accurate IMHO, since you're buffering necko notifications, not anything related to IPDL.  Third, this patch is lacking summary comment(s) describing the invariants that require us to queue notifications in certain circumstances (bonus points for making snarky remarks about sync XHR and extensions spinning the XPCOM event loop).

Comment 10

[Jason Duell]

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

Looks good--I made a bunch of changes.  Attaching patch for jdm's perusal before I check in.

Comment 11

[Jason Duell]

Attachment: Made some changes to main patch

Changed 'buffer' names to 'queue' per cjones.

Added comment about why we buffer (I mean, queue) in .h file

Made some functions inline: ShouldBuffer, EnsureUnbuffered, BeginIPDLBuffering.  (Remember, an inline version of a function that just does a couple lines of code is probably an order of magnitude faster.)

We never return false from IPDL handlers, so remove lots of chains of passing bool return values (which wouldn't have gotten back to IPDL when we buffer anyway).

Added AutoChildEventEnqueuer class that automatically turns on queueing in scope, and flushes queue on destruction.

I couldn't figure out why you need EnsureUnbuffered():  you only call it after you've already returned if state != PHASE_UNBUFFERED.  Removed.

Added buffering logic to OnStartRequest, which was only function that we never buffered.  Not sure if it's needed, but won't do any harm if not.

Comment 12

[Jason Duell]

http://hg.mozilla.org/projects/electrolysis/rev/fe54d69820f8