Closed Bug 584604 Opened 11 years ago Closed 11 years ago

Clean up HTTP buffering of incoming IPDL necko messages

Categories

(Core :: IPC, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Tracking Status
fennec 2.0a1+ ---

People

(Reporter: dougt, Assigned: jdm)

References

Details

Attachments

(3 files, 5 obsolete files)

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)
Assignee: nobody → josh
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"?
Depends on: 564351
Blocks: 558801
tracking-fennec: --- → ?
Attached patch Patch (obsolete) — Splinter Review
All comments addressed.
Attachment #463305 - Flags: review?(jones.chris.g)
Attachment #463305 - Flags: review?(jduell.mcbugs)
Attached patch Progress test changes (obsolete) — Splinter Review
Progress test changes.
Attachment #463307 - Flags: review?(jduell.mcbugs)
Attached patch Sync XHR test (obsolete) — Splinter Review
Now with more sync XHR testing.
Attachment #463308 - Flags: review?(jduell.mcbugs)
Attachment #463308 - Attachment is patch: true
Attachment #463308 - Attachment mime type: application/octet-stream → text/plain
Attached patch Progress test changes (obsolete) — Splinter Review
Same as the previous progress test, but now with fewer leakage reports from xpcshell.
Attachment #463307 - Attachment is obsolete: true
Attachment #463311 - Flags: review?(jduell.mcbugs)
Attachment #463307 - Flags: review?(jduell.mcbugs)
Attached patch Patch (obsolete) — Splinter Review
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.
Attachment #463305 - Attachment is obsolete: true
Attachment #463625 - Flags: review?(jones.chris.g)
Attachment #463625 - Flags: review?(jduell.mcbugs)
Attachment #463305 - Flags: review?(jones.chris.g)
Attachment #463305 - Flags: review?(jduell.mcbugs)
Attached patch Sync XHR testSplinter Review
Updated xhr test.  It turns out the previous version was completely bogus, in all possible senses of the word.
Attachment #463308 - Attachment is obsolete: true
Attachment #463626 - Flags: review?(jduell.mcbugs)
Attachment #463308 - Flags: review?(jduell.mcbugs)
The previous progress test changes actually broke all testing.  Woo.
Attachment #463311 - Attachment is obsolete: true
Attachment #463627 - Flags: review?(jduell.mcbugs)
Attachment #463311 - Flags: review?(jduell.mcbugs)
Blocks: 536321
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).
Attachment #463625 - Flags: review?(jones.chris.g) → review+
Blocks: 536317
tracking-fennec: ? → 2.0a1+
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.
Attachment #463625 - Flags: review?(jduell.mcbugs) → review+
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.
Attachment #463625 - Attachment is obsolete: true
Attachment #464548 - Flags: review+
http://hg.mozilla.org/projects/electrolysis/rev/fe54d69820f8
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Attachment #463626 - Flags: review?(jduell.mcbugs)
Attachment #463627 - Flags: review?(jduell.mcbugs)
You need to log in before you can comment on or make changes to this bug.