598076 - Generalize IPDL queueing for e10s protocols

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Josh Matthews [:jdm]]

The queueing is pretty easily separated into an EventQueue class that protocols can inherit from.

Comment 1

[Josh Matthews [:jdm]]

Attachment: Create EventQueue class to allow multiple necko protocols to reuse IPDL queueing code.

Comment 2

[Josh Matthews [:jdm]]

Attachment: Update HTTPChannelChild to use new general event queueing infrastructure.

Comment 3

[Jason Duell]

Comment on attachment 476866 [details] [diff] [review]
Create EventQueue class to allow multiple necko protocols to reuse IPDL queueing code.

>+class EventQueue
>+{
>+ public:
>+  EventQueue() : mQueuePhase(PHASE_UNQUEUED) {}
>+  virtual ~EventQueue();

Do you need an empty function body for the destructor, too?  Or = 0?  (In the absence of one of those, aren't you required to provide a definition somewhere?) We don't really need the destructor to be virtual, do we?

>+ protected:

>+  // Workaround for Necko re-entrancy dangers. We buffer IPDL messages in a
>+  // queue if still dispatching previous one(s) to listeners/observers.
>+  // Otherwise synchronous XMLHttpRequests and/or other code that spins the
>+  // event loop (ex: IPDL rpc) could cause listener->OnDataAvailable (for
>+  // instance) to be called before mListener->OnStartRequest has completed.

Let's move this comment up to just above the EventQueue class definition, so it describes the whole class.

>+  virtual void BeginEventQueueing();
>+  virtual void EndEventQueueing();
>+  virtual void EnqueueEvent(ChannelEvent* callback);
>+  virtual bool ShouldEnqueue();

You're declaring these as inline, which I don't *think* plays so well with "virtual" (unless compiler optimizers are reliably smart: they often aren't).  If we don't actually need them to be virtual (and it doesn't look like it AFAICT), let's not use it.

Actually, nothing in this class really needs to be virtual, does it?  We're never in a place where we have a pointer to the base type and don't know the subclass.  If you want to be C++ geeky, you should be able to write this using a template.  Would only be a very minor perf win (non-virtual call to FlushQ), so don't bother, unless you're into that sort of thing...

Otherwise looks good.

Comment 4

[Jason Duell]

Comment on attachment 476867 [details] [diff] [review]
Update HTTPChannelChild to use new general event queueing infrastructure.

>@@ -74,6 +75,7 @@
>                        , public nsIApplicationCacheChannel
>                        , public nsIAsyncVerifyRedirectCallback
>                        , public nsIAssociatedContentSecurity
>+                       , public EventQueue

Hmm, mildly allergic reaction all of a sudden to naming it plain old
"EventQueue", instead of "ChannelEventQueue" or something.  I guess that's why
we have namespaces (though it can make navigating mxr trickier if someone winds
up reusing the name later).  I'll let you decide.

> inline bool
> HttpChannelChild::ShouldEnqueue()
> {
>-  return mQueuePhase != PHASE_UNQUEUED || mSuspendCount;
>+  return EventQueue::ShouldEnqueue() || mSuspendCount;

Note that this doesn't require ShouldEnqueue to be virtual.

Comment 5

[Josh Matthews [:jdm]]

Attachment: Create EventQueue class to allow multiple necko protocols to reuse IPDL queueing code.

Comment 6

[Josh Matthews [:jdm]]

Attachment: Update HTTPChannelChild to use new general event queueing infrastructure.

Comment 7

[Josh Matthews [:jdm]]

All comments addressed.

Comment 8

[Josh Matthews [:jdm]]

Attachment: Update HTTPChannelChild to use new general event queueing infrastructure.

Bitrot.

Comment 9

[Jason Duell]

This should be marked a fennec blocker at same priority as FTP (bug 536289) since it blocks it.

I vote we change this and FTP to blocking-b2

Comment 10

[Josh Matthews [:jdm]]

http://hg.mozilla.org/mozilla-central/rev/f93c9ecd5f28
http://hg.mozilla.org/mozilla-central/rev/651854be26ec