1263991 - [FlyWeb] Make publishServer support e10s

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Currently the publishServer API doesn't work in child processes. We need to fix that.

Here's the approximate design that I think would work.

* Create a new ipdl protocol which has functions for the the network-related
  functions on FlyWebPublishedServer. This protocol should be able to send the
  following messages from child to parent: OnFetchResponse, OnWebSocketAccept
  and OnWebSocketResponse.
  And the following messages from parent to child to parent: OnRequest,
  OnWebSocket, OnServerClose and Init.
* Remove the 'port' property on FlyWebPublishedServer as it just makes things
  more complicated for now.
* When a FlyWebPublishedServer is created in the child, we create an ipdl
  protocol object and send an Init message with enough information that we in
  the parent can create a HttpServer and call the FlyWebPublishedServer ctor.
  Note that in the parent we don't need the aOwner argument and should leave it
  as null. We'll have to modify some code to make this ok.
* When OnRequest/OnWebSocket/OnServerClose is called in the parent, send a
  message to the child with the relevant information.
* When OnFetchResponse/OnWebSocketAccept/OnWebSocketResponse is called in the
  child, send a message to the parent with the relevant information.
  The main part that I haven't looked into is how to send the
  InternalResponse/InternalRequest objects, and their bodies, across the process
  boundary. This is especially tricky since the HttpServer API is very based on
  object identity of InternalRequest objects. We might want to change that to
  use some form of numeric IDs instead.

To make the websocket server work we need to also:

* Write new ipdl protocol for nsITransportProvider. This protocol does not need
  to have any messages other than ctor and dtor.
* Write new implementation of nsITransportProvider specifically for child
  processes. This implementation should hold on to the ipdl protocol object
  above.
* The parent part of the ipdl protocol should hold a reference to the
  HttpServer.
* When the publishedServer ipdl protocol proxies the OnWebSocket call from the
  parent to the child, we send along the nsITransportProvider ipdl protocol
  object to the child and hold it in the FlyWebPublishedServer implementation
  in the child. Note that we still haven't extracted a nsITransportProvider
  in the parent yet at this point. The parent side just holds on to the
  HttpServer at this point.
* When FlyWebPublishedServer::OnWebSocketAccept is called in the child, let
  the child create a WebSocket object using the child-nsITransportProvider
  object.
* Make WebSocketChannelChild::AsyncOpen send the nsITransportProvider ipdl
  protocol object along with the other parameters to SendAsyncOpen.
* In the parent we can extract an nsITransportProvider object from the
  HttpServer using the protocol object.
* Make WebSocketChannelParent::RecvAsyncOpen call SetServerParameters if it
  receives a nsITransportProvider.

Comment 1

[Kannan Vijayan [:djvj]]

Attachment: publish-server-ipc.patch

Initial tabs at writing a FlyWebPublishedServer IPDL file and implementing it.  This is just the overall scaffolding code, no implementation yet.

Comment 2

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Part 1: Remove some properties that are not needed right now

Reduce the API surface to make e10s simpler. We might need these later, but if so we can add them at that point.

Comment 3

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Part 2: Enable launching a server in the parent process

This splits up the FlyWebPublishedServer class such that we have a baseclass (FlyWebPublishedServer) and two subclasses (FlyWebPublishedServerChild and FlyWebPublishedServerImpl).

FlyWebPublishedServerChild is for implementing the publishServer API in the child process, and FlyWebPublishedServerImpl is for implementing the publishServer API in the running in the child process and FlyWebPublishedServerImpl is for 

One of the two subclass classes is for running in the parent process (FlyWebPublishedServerImpl), and one is for child processes (FlyWebPublishedServerChild).

When the FlyWebPublishedServerChild is instantiated in the child process, it uses an ipdl protocol to create an actor in the parent process which then launches the parent subclass.

Comment 4

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Part 3: Support incoming and outgoing requests/responses

Send incoming requests from the parent to the child, and outgoing responses from the child to the parent.

This patch does not add support for response bodies though, which means that e10s is still not fully working.

Comment 5

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Part 4: Support response body

This adds support for the response body as well, meaning that http mostly works in e10s. Only thing missing is request bodies.

We should also switch the cache code over to using these new InternalRequest/InternalResponse ipdl structs, but there is enough cache specific functionality in the existing code, that I'd rather do that as a followup.

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8758634 [details] [diff] [review]
Part 3: Support incoming and outgoing requests/responses

Review of attachment 8758634 [details] [diff] [review]:
-----------------------------------------------------------------

You didn't ask me to look at this one, but I did so I could understand the other patches.  f=me with comments addressed.

::: dom/fetch/FetchTypes.ipdlh
@@ +11,5 @@
> +using RequestCredentials from "mozilla/dom/cache/IPCUtils.h";
> +using RequestMode from "mozilla/dom/cache/IPCUtils.h";
> +using ResponseType from "mozilla/dom/cache/IPCUtils.h";
> +using RequestRedirect from "mozilla/dom/cache/IPCUtils.h";
> +using RequestCache from "mozilla/dom/cache/IPCUtils.h";

You should just move all these enum param traits into a fetch header and then make cache use that.  Except for the cache::namespace type you can literally just move the file.

::: dom/fetch/InternalHeaders.h
@@ +152,5 @@
> +
> +void
> +InternalHeadersToIPC(InternalHeaders* aHeaders,
> +                     nsTArray<HeadersEntry>& aIPCHeaders,
> +                     HeadersGuardEnum& aGuard);

Can this be a method on InternalHeaders?  Just ir->ToIPC()?  I don't understand why this is an external function like this.

@@ +156,5 @@
> +                     HeadersGuardEnum& aGuard);
> +void
> +IPCToInternalHeaders(const nsTArray<HeadersEntry>& aHeadersEntryList,
> +                     HeadersGuardEnum aGuard,
> +                     InternalHeaders* aHeaders);

Similarly, this should probably be a static method on InternalHeaders.  Something like InternalHeaders::FromIPC().

::: dom/fetch/InternalRequest.cpp
@@ +415,5 @@
> +  RefPtr<InternalRequest> request = new InternalRequest(aIPCRequest.urls()[0]);
> +
> +  for (uint32_t i = 1; i < aIPCRequest.urls().Length(); ++i) {
> +    request->AddURL(aIPCRequest.urls()[i]);
> +  }

We should probably assert that at least one URL as added here.

::: dom/fetch/InternalRequest.h
@@ +516,5 @@
> +void
> +InternalRequestToIPC(InternalRequest* aRequest,
> +                     IPCInternalRequest* aIPCRequest);
> +already_AddRefed<InternalRequest>
> +IPCToInternalRequest(const IPCInternalRequest& aIPCRequest);

Same comments about moving these into the class instead of external functions.

::: dom/fetch/InternalResponse.h
@@ +308,5 @@
> +void
> +InternalResponseToIPC(InternalResponse* aResponse,
> +                     IPCInternalResponse* aIPCResponse);
> +already_AddRefed<InternalResponse>
> +IPCToInternalResponse(const IPCInternalResponse& aIPCResponse);

Same comments about moving these into the class instead of external functions.

::: dom/flyweb/FlyWebPublishedServer.cpp
@@ +264,5 @@
> +{
> +  LOG_I("FlyWebPublishedServerChild::RecvFetchRequest(%p)", this);
> +
> +  RefPtr<InternalRequest> request = IPCToInternalRequest(aRequest);
> +  ErrorResult result;

This ErrorResult is not used?

@@ +290,5 @@
> +    LOG_I("FlyWebPublishedServerChild::OnFetchResponse(%p) - No actor!", this);
> +    return;
> +  }
> +
> +  uint64_t id = mPendingRequests.Get(aRequest);

Seems you should probably assert that you actually found the request here?

@@ +295,5 @@
> +  IPCInternalResponse ipcResp;
> +  InternalResponseToIPC(aResponse, &ipcResp);
> +  Unused << SendFetchResponse(ipcResp, id);
> +
> +  mPendingRequests.Remove(aRequest);

Why not remove this above?  Seems safer to update state immediately in case of re-entry (although I admit thats unlikely here).

@@ +357,5 @@
>  
> +  mozPromise->Then(
> +    AbstractThread::MainThread(),
> +    __func__,
> +    [this, self] (FlyWebPublishedServer* aServer) {

I don't really understand whats going on here, but can you safely capture a stack variable like this if mozPromise->Then() is truly async?

@@ +388,5 @@
> +
> +  nsAutoString type;
> +  aEvent->GetType(type);
> +  if (type.EqualsLiteral("close")) {
> +    Unused << SendServerClose();

This can fail if the child process is dead, but doesn't look like there is anything for you to do here in that case.

@@ +397,5 @@
> +    mPendingRequests.Put(id, request);
> +
> +    IPCInternalRequest ipcReq;
> +    InternalRequestToIPC(request, &ipcReq);
> +    Unused << SendFetchRequest(ipcReq, id);

You should check for failure here since the child process could be gone.  In that case you probably want to cleanup your mPendingRequests.

@@ +411,5 @@
> +  RefPtr<InternalResponse> response = IPCToInternalResponse(aResponse);
> +
> +  mPublishedServer->OnFetchResponse(mPendingRequests.GetWeak(aRequestId),
> +                                    response);
> +  mPendingRequests.Remove(aRequestId);

MOZ_CRASH() if the requestID is not present in mPendingRequests?  That probably means someone is trying to spoof the response across the IPC link.

Comment 7

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8758639 [details] [diff] [review]
Part 4: Support response body

Review of attachment 8758639 [details] [diff] [review]:
-----------------------------------------------------------------

Overall looks good, but I have a suggestion for improving the API.

::: dom/fetch/InternalResponse.h
@@ +320,4 @@
>  void
>  InternalResponseToIPC(InternalResponse* aResponse,
> +                      IPCInternalResponse* aIPCResponse,
> +                      M* aManager,

nit: I personally think its nicer to keep the body definition in the cpp file.  You can expose two serialization functions with PContent and PBackground in the h file.  Then in the cpp file you have these call a templated inner function.

::: dom/flyweb/FlyWebPublishedServer.cpp
@@ +294,5 @@
>  
>    uint64_t id = mPendingRequests.Get(aRequest);
>    IPCInternalResponse ipcResp;
> +  UniquePtr<mozilla::ipc::AutoIPCStream> autoStream;
> +  InternalResponseToIPC(aResponse, &ipcResp, Manager(), autoStream);

As an API goes I don't love passing in a UniquePtr<AutoIPCStream> and maybe getting a stream out.

What do you think about instead making a AutoIPCInternalResponse wrapper class?  This would internally own the IPCInternalResponse and the AutoIPCStream.  You would use it something like:

  AutoIPCInternalResponse ipcResp;
  ipcResp->Serialize(aResponse, Manager());
  Unused << SendFetchResponse(ipcResp.TakeValue(), id);

It would add a bit more code for the new Auto wrapper class, but would make the API a lot safer to use.

Comment 8

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

> You should just move all these enum param traits into a fetch header and
> then make cache use that.  Except for the cache::namespace type you can
> literally just move the file.

Done. It would be really nice if webidl allowed annotating dictionaries and enums as "ipctransferable" which would cause us to generate the necessary (de)serialization templates.

> ::: dom/fetch/InternalHeaders.h
> @@ +152,5 @@
> > +
> > +void
> > +InternalHeadersToIPC(InternalHeaders* aHeaders,
> > +                     nsTArray<HeadersEntry>& aIPCHeaders,
> > +                     HeadersGuardEnum& aGuard);
> 
> Can this be a method on InternalHeaders?  Just ir->ToIPC()?  I don't
> understand why this is an external function like this.

Not quite sure why I didn't do this originally. Done now and it produces much nicer code.

I added ToIPC() methods on InternalHeaders/InternalRequest/InternalResponse
I also added a ctor which takes an ipdl struct for InternalHeaders/InternalRequest.

Sadly making InternalResponse use a ctor doesn't currently work since we have to call CORSResponse etc which generates a separate object. So added a static FromIPC on InternalResponse.

> @@ +357,5 @@
> >  
> > +  mozPromise->Then(
> > +    AbstractThread::MainThread(),
> > +    __func__,
> > +    [this, self] (FlyWebPublishedServer* aServer) {
> 
> I don't really understand whats going on here, but can you safely capture a
> stack variable like this if mozPromise->Then() is truly async?

The above lambda catches 'self' by value. Which means that a new RefPtr is created and kept alive until the lambda dies.

> @@ +397,5 @@
> > +    mPendingRequests.Put(id, request);
> > +
> > +    IPCInternalRequest ipcReq;
> > +    InternalRequestToIPC(request, &ipcReq);
> > +    Unused << SendFetchRequest(ipcReq, id);
> 
> You should check for failure here since the child process could be gone.  In
> that case you probably want to cleanup your mPendingRequests.

At this point we know that the main thread has not yet been informed of the child process dying since there's an actor-alive check above.

If the child process does die, we'll tear down all related objects as quickly as we can.

So not much benefit to calling cleanup for each sent message.

> @@ +411,5 @@
> > +  RefPtr<InternalResponse> response = IPCToInternalResponse(aResponse);
> > +
> > +  mPublishedServer->OnFetchResponse(mPendingRequests.GetWeak(aRequestId),
> > +                                    response);
> > +  mPendingRequests.Remove(aRequestId);
> 
> MOZ_CRASH() if the requestID is not present in mPendingRequests?  That
> probably means someone is trying to spoof the response across the IPC link.

We shouldn't MOZ_CRASH, since this runs in the parent. But I added code to kill the child.

Comment 9

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Part 3: Support incoming and outgoing requests/responses

Comment 10

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

(In reply to Ben Kelly [:bkelly] from comment #7)
> Comment on attachment 8758639 [details] [diff] [review]
> Part 4: Support response body
> 
> Review of attachment 8758639 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Overall looks good, but I have a suggestion for improving the API.
> 
> ::: dom/fetch/InternalResponse.h
> @@ +320,4 @@
> >  void
> >  InternalResponseToIPC(InternalResponse* aResponse,
> > +                      IPCInternalResponse* aIPCResponse,
> > +                      M* aManager,
> 
> nit: I personally think its nicer to keep the body definition in the cpp
> file.  You can expose two serialization functions with PContent and
> PBackground in the h file.  Then in the cpp file you have these call a
> templated inner function.

Actually, C++ has a feature which enables keeping the template implementation in the C++ file and declare which template parameters it should be complied for.

> ::: dom/flyweb/FlyWebPublishedServer.cpp
> @@ +294,5 @@
> >  
> >    uint64_t id = mPendingRequests.Get(aRequest);
> >    IPCInternalResponse ipcResp;
> > +  UniquePtr<mozilla::ipc::AutoIPCStream> autoStream;
> > +  InternalResponseToIPC(aResponse, &ipcResp, Manager(), autoStream);
> 
> As an API goes I don't love passing in a UniquePtr<AutoIPCStream> and maybe
> getting a stream out.

The UniquePtr<AutoIPCStream> argument is just so that I can call .TakeValue() on it. I don't really understand why the call to .TakeValue() is needed but I'm guessing it does some form of cleanup?

> What do you think about instead making a AutoIPCInternalResponse wrapper
> class?  This would internally own the IPCInternalResponse and the
> AutoIPCStream.  You would use it something like:
> 
>   AutoIPCInternalResponse ipcResp;
>   ipcResp->Serialize(aResponse, Manager());
>   Unused << SendFetchResponse(ipcResp.TakeValue(), id);
> 
> It would add a bit more code for the new Auto wrapper class, but would make
> the API a lot safer to use.

Having used something similar for IPCStream just now, i don't really find this pattern great to work with. What happens if someone wants to embed an InternalResponse inside another struct?

I'd rather that we figure out a nicer solution for IPCStream which enables embedding it in messages without jumping through so many hoops.

Comment 11

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Part 4: Support response body

Comment 12

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Jonas Sicking (:sicking) from comment #10)
> > ::: dom/flyweb/FlyWebPublishedServer.cpp
> > @@ +294,5 @@
> > >  
> > >    uint64_t id = mPendingRequests.Get(aRequest);
> > >    IPCInternalResponse ipcResp;
> > > +  UniquePtr<mozilla::ipc::AutoIPCStream> autoStream;
> > > +  InternalResponseToIPC(aResponse, &ipcResp, Manager(), autoStream);
> > 
> > As an API goes I don't love passing in a UniquePtr<AutoIPCStream> and maybe
> > getting a stream out.

The whole point of the Auto* class is to cleanup the internal actors if necessary.  So if you hit an error condition and don't actually send it across the IPC layer you want to Send__delete__() those actors you just created.

In addition, if you end up sending a PSendStream, the TakeValue() is where we trigger the start of the stream copying.  Its an async process that should not complete before you send the PSendStream handle across to the other side.  So it shouldn't start until you send the IPCStream struct across.

> What happens if someone wants to embed an InternalResponse inside another struct?

You can do this the same way AutoIPCStream supports it.  You pass the struct to fill in to the AutoIPCInternalResponse constructor.  I just didn't include that in my short example.

Letting AutoIPInternalResponse own the IPCInternalStream struct is inheritently safer, though.  You don't have to worry about the IPC stream moving in memory or getting destroyed.  I ran into a problem with that in dom/cache when I had an array of IPCStream structures get realloc'd.

I'm happy to look at other patterns.  But this was the best I could find that balanced:

1) Correct actor cleanup handling
2) Small API surface area
3) Flexible enough to handle complex cases like dom/cache

Anyway, we can do this stuff in a follow-up.  I don't think there is a functional problem in your current code.  Might be worth a TODO comment to consider an AutoIPCInternalResponse class in the future.

Comment 13

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8759624 [details] [diff] [review]
Part 4: Support response body

Review of attachment 8759624 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks.  r=me with comments addressed.

::: dom/fetch/InternalResponse.cpp
@@ +110,2 @@
>  {
>    MOZ_ASSERT(!mURLList.IsEmpty());

MOZ_ASSERT(!aAutoStream)?  Do you expect an existing AutoIPCStream to get passed here ever?

::: dom/fetch/InternalResponse.h
@@ +40,5 @@
> +  template<typename M>
> +  void
> +  ToIPC(IPCInternalResponse* aIPCResponse,
> +        M* aManager,
> +        UniquePtr<mozilla::ipc::AutoIPCStream>& aAutoStream);

Can you add a documenting comment here?  It may not be clear that the caller must make a aAutoStream->TakeOptionalValue() when they pass their IPCInternalResponse across IPC.

Also, please include a TODO or XXX comment that we might want to provide an Auto* style helper class here.

Comment 14

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Part 5: Add e10s-friendly internal API for accepting websocket connections

Comment 15

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Part 6: Add WebSocket support

Comment 16

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 8760036 [details] [diff] [review]
Part 6: Add WebSocket support

Baku, can you review the non-/netwerk changes.

Michal, can you review the /netwerk changes. They go hand-in-hand with the websocket changes you reviewed recently, just adds e10s support to the same mechanisms.

Comment 17

[Andrea Marchesini [:baku]]

Comment on attachment 8758633 [details] [diff] [review]
Part 2: Enable launching a server in the parent process

Review of attachment 8758633 [details] [diff] [review]:
-----------------------------------------------------------------

I don't understand the memory management of these Child and Parent actors.

::: dom/base/Navigator.cpp
@@ +1631,5 @@
>      return nullptr;
>    }
>  
> +  RefPtr<FlyWebPublishPromise> mozPromise =
> +    service->PublishServer(aName, aOptions, mWindow);

It seems that a mozPromise is always returned.
MOZ_ASSERT(mozPromise)

::: dom/flyweb/FlyWebPublishedServer.cpp
@@ +13,3 @@
>  #include "mozilla/Preferences.h"
>  #include "nsGlobalWindow.h"
> +#include "mozilla/unused.h"

these headers were in alphabetic order...

@@ +41,3 @@
>  {
> +  // Make sure to unregister to avoid dangling pointers. Use the LastRelease
> +  // hook rather than dtor since calling virtual functions during dtor 

extra space.

@@ +60,3 @@
>    // Unregister from server.
>    if (mIsRegistered) {
>      FlyWebService::GetOrCreate()->UnregisterServer(this);

GetOrCreate? Why do we need to create if we have a publisher registered?
Can you do:

FlyWebService* service = FlyWebService::Get();
MOZ_ASSERT(service);
service->UnregisterServer(this);

@@ +231,4 @@
>  {
> +  LOG_I("FlyWebPublishedServerChild::FlyWebPublishedServerChild(%p)", this);
> +
> +  ContentChild::GetSingleton()->

Eventually we will migrate it to PBackground and ServiceWorkers...

@@ +231,5 @@
>  {
> +  LOG_I("FlyWebPublishedServerChild::FlyWebPublishedServerChild(%p)", this);
> +
> +  ContentChild::GetSingleton()->
> +    SendPFlyWebPublishedServerConstructor(this,

This can fail.

@@ +235,5 @@
> +    SendPFlyWebPublishedServerConstructor(this,
> +                                          PromiseFlatString(aName),
> +                                          aOptions);
> +
> +  NS_ADDREF_THIS();

I don't see any RELEASE(). Plus, I guess you can use this into the ContentChild/Parent code.

@@ +241,5 @@
> +
> +bool
> +FlyWebPublishedServerChild::RecvServerReady(const nsresult& aStatus)
> +{
> +  LOG_I("FlyWebPublishedServerChild::RecvServerReady(%p)", this);

MOZ_ASSERT(!amActorDestroyed);

@@ +250,5 @@
> +
> +bool
> +FlyWebPublishedServerChild::RecvServerClose()
> +{
> +  LOG_I("FlyWebPublishedServerChild::RecvServerClose(%p)", this);

MOZ_ASSERT(!mActorDestroyed);

@@ +290,5 @@
> +
> +void
> +FlyWebPublishedServerChild::Close()
> +{
> +  LOG_I("FlyWebPublishedServerChild::Close(%p)", this);

MOZ_ASSERT(!mActorDestroyed);

@@ +350,5 @@
> +
> +bool
> +FlyWebPublishedServerParent::Recv__delete__()
> +{
> +  LOG_I("FlyWebPublishedServerParent::Recv__delete__(%p)", this);

MOZ_ASSERT(!mActorDestroyed);

::: dom/flyweb/FlyWebPublishedServer.h
@@ +112,5 @@
> +                            const FlyWebPublishOptions& aOptions);
> +
> +  NS_DECL_ISUPPORTS_INHERITED
> +
> +  int32_t Port()

int32_t Port() const

@@ +116,5 @@
> +  int32_t Port()
> +  {
> +    return mHttpServer ? mHttpServer->GetPort() : 0;
> +  }
> +  void GetCertKey(nsACString& aKey) {

void GetCertKey(nsACString& aKey) const {... maybe?

@@ +191,5 @@
> +
> +private:
> +  virtual ~FlyWebPublishedServerChild() {}
> +
> +  bool mActorDestroyed;

You don't use it.

@@ +211,5 @@
> +  Recv__delete__() override;
> +
> +  virtual ~FlyWebPublishedServerParent() {}
> +
> +  bool mActorDestroyed;

You don't use it.

::: dom/flyweb/FlyWebService.cpp
@@ +804,5 @@
>  
>  ErrorResult
>  FlyWebService::Init()
>  {
> +  if (XRE_GetProcessType() == GeckoProcessType_Content) {

Write a comment about this.

::: dom/ipc/ContentChild.cpp
@@ +1698,5 @@
> +}
> +
> +bool
> +ContentChild::DeallocPFlyWebPublishedServerChild(PFlyWebPublishedServerChild* aActor)
> +{

Why no deallocation?

::: dom/ipc/ContentParent.cpp
@@ +4160,5 @@
> +}
> +
> +bool
> +ContentParent::DeallocPFlyWebPublishedServerParent(PFlyWebPublishedServerParent* aActor)
> +{

No deallocation? This seems a bug.

Comment 18

[Andrea Marchesini [:baku]]

Comment on attachment 8759614 [details] [diff] [review]
Part 3: Support incoming and outgoing requests/responses

Review of attachment 8759614 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/fetch/InternalHeaders.cpp
@@ +11,5 @@
>  #include "nsCharSeparatedTokenizer.h"
>  #include "nsContentUtils.h"
>  #include "nsNetUtil.h"
>  #include "nsReadableUtils.h"
> +#include "mozilla/dom/FetchTypes.h"

move it to the other 'mozilla/dom/' headers

::: dom/fetch/InternalRequest.cpp
@@ +12,5 @@
>  
>  #include "mozilla/ErrorResult.h"
>  #include "mozilla/dom/ScriptSettings.h"
>  #include "mozilla/dom/workers/Workers.h"
> +#include "mozilla/dom/FetchTypes.h"

alphabetic order.

@@ +124,5 @@
>  }
>  
>  void
> +InternalRequest::ToIPC(IPCInternalRequest* aIPCRequest)
> +{

IPCInternalRequest& aIPCRequest
o
MOZ_ASSERT(aIPCRequest)

::: dom/fetch/InternalResponse.cpp
@@ +79,5 @@
>  }
>  
> +void
> +InternalResponse::ToIPC(IPCInternalResponse* aIPCResponse)
> +{

similar to the Request::ToIPC

::: dom/flyweb/FlyWebPublishedServer.cpp
@@ +295,5 @@
> +  MOZ_ASSERT(id);
> +  mPendingRequests.Remove(aRequest);
> +
> +  IPCInternalResponse ipcResp;
> +  aResponse->ToIPC(&ipcResp);

ok... just pass the reference. and not &ipcResp. Any reason why you do this?

@@ +390,5 @@
> +
> +  nsAutoString type;
> +  aEvent->GetType(type);
> +  if (type.EqualsLiteral("close")) {
> +    Unused << SendServerClose();

return NS_OK;
}

MOZ_ASSERT(type.EqualsLiteral("fetch"));
....

Comment 19

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Andrea Marchesini (:baku) from comment #18)
> ::: dom/flyweb/FlyWebPublishedServer.cpp
> > +  IPCInternalResponse ipcResp;
> > +  aResponse->ToIPC(&ipcResp);
> 
> ok... just pass the reference. and not &ipcResp. Any reason why you do this?

Its pretty common style to take the reference of a struct being filled in as an out variable, no?

Comment 20

[Michal Novotny [:michal]]

Comment on attachment 8760036 [details] [diff] [review]
Part 6: Add WebSocket support

Review of attachment 8760036 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/protocol/websocket/WebSocketChannelChild.cpp
@@ +435,5 @@
>  {
>    LOG(("WebSocketChannelChild::AsyncOpen() %p\n", this));
>  
>    MOZ_ASSERT(NS_IsMainThread(), "not main thread");
> +  MOZ_ASSERT((aURI || mIsServerSide) && aListener && !mListenerMT,

(aURI && !mIsServerSize) || (!aURI && mIsServerSize) would be IMO more correct

Comment 21

[Kannan Vijayan [:djvj]]

Comment on attachment 8760035 [details] [diff] [review]
Part 5: Add e10s-friendly internal API for accepting websocket connections

Review of attachment 8760035 [details] [diff] [review]:
-----------------------------------------------------------------

Looks ok.  My only thoughts, after reviewing this, is to wonder if inheritance is really the right mechanism to use to share code between PublishedServerImpl and PublishedServerChild.  But I think that commentary is probably better suited to feedback on the main patch.

Comment 22

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

(In reply to Andrea Marchesini (:baku) from comment #17)
> Comment on attachment 8758633 [details] [diff] [review]
> Part 2: Enable launching a server in the parent process
> 
> Review of attachment 8758633 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I don't understand the memory management of these Child and Parent actors.

Yeah, the memory management is all sorts of messed up. Good catch!

I'm testing a new version where we let "ipdl" keep strong references to both the child and parent actors. These references are released when the actors are destroyed.

This means that these objects can't be GCed until the server functionality is shut down. But that's the behavior we want anyway so that's fine.

When the user navigates away from a page, FlyWebService will call .close() on any servers running in that page. So nothing should get leaked.

There are other potential solutions, but this seemed like a relatively safe and simple one.

> @@ +231,4 @@
> >  {
> > +  LOG_I("FlyWebPublishedServerChild::FlyWebPublishedServerChild(%p)", this);
> > +
> > +  ContentChild::GetSingleton()->
> 
> Eventually we will migrate it to PBackground and ServiceWorkers...

Yeah. There's still some stuff that only runs on the main thread in the parent part here. Specifically some of the mDNS stuff. And I'm also worried that some of the necko server-socket stuff hasn't been hardened to run off-main-thread.

But I agree, we eventually will want to use PBackground.

> @@ +231,5 @@
> >  {
> > +  LOG_I("FlyWebPublishedServerChild::FlyWebPublishedServerChild(%p)", this);
> > +
> > +  ContentChild::GetSingleton()->
> > +    SendPFlyWebPublishedServerConstructor(this,
> 
> This can fail.

Under what circumstances can this fail given that the client actor creation can never fail (since it is 'this')?

Looking at the generated PContentChild::SendPFlyWebPublishedServerConstructor, the only failure is mChannel->Send(...). It seems like in that case there's little hope that the child process will work functionally.

It's actually super annoying to deal with failure here since MozPromise doesn't support resolving a promise before Then-listeners have been registered.

> @@ +241,5 @@
> > +
> > +bool
> > +FlyWebPublishedServerChild::RecvServerReady(const nsresult& aStatus)
> > +{
> > +  LOG_I("FlyWebPublishedServerChild::RecvServerReady(%p)", this);
> 
> MOZ_ASSERT(!mActorDestroyed);

Is it really important to assert that the actor hasn't been destroyed when there are *incoming* messages? Even if we have some bug causing that situation to happen, I would think nothing bad happens until you try to *send* messages from a destroyed actor.

I've tried to protect all calls to Send* by first checking mActorDestroyed, is that enough?

I'm still happy to add MOZ_ASSERT(!mActorDestroyed) assertions for all incoming messages if that's what we generally do in ipdl protocols, but it seems to add more noise than help to me.

> ::: dom/flyweb/FlyWebPublishedServer.h
> @@ +112,5 @@
> > +                            const FlyWebPublishOptions& aOptions);
> > +
> > +  NS_DECL_ISUPPORTS_INHERITED
> > +
> > +  int32_t Port()
> 
> int32_t Port() const

Is there a reason to worry about supporting operations on const DOM objects? Those generally do not appear.

I don't feel strongly though, and I'm happy to add const on this and GetCertKey() if you prefer.

> @@ +191,5 @@
> > +
> > +private:
> > +  virtual ~FlyWebPublishedServerChild() {}
> > +
> > +  bool mActorDestroyed;
> 
> You don't use it.

It's not used very much yet since we this doesn't send a lot of messages in either direction yet. But it's used in FlyWebPublishedServerChild::Close where we send the __delete__ message.

> @@ +211,5 @@
> > +  Recv__delete__() override;
> > +
> > +  virtual ~FlyWebPublishedServerParent() {}
> > +
> > +  bool mActorDestroyed;
> 
> You don't use it.

Same here, we just don't send a lot of messages until the Part 3 patch. But it is used in the lambdas in FlyWebPublishedServerParent::FlyWebPublishedServerParent where we send messages from the parent to the child when the promise resolves.

Comment 23

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Part 2: Enable launching a server in the parent process

Comment 24

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

(In reply to Andrea Marchesini (:baku) from comment #18)
> @@ +295,5 @@
> > +  MOZ_ASSERT(id);
> > +  mPendingRequests.Remove(aRequest);
> > +
> > +  IPCInternalResponse ipcResp;
> > +  aResponse->ToIPC(&ipcResp);
> 
> ok... just pass the reference. and not &ipcResp. Any reason why you do this?

Passing a pointer when an argument is mutated seems to be the convention that the generated ipdl code uses, for example for Dealloc*, so I wanted to follow that convention.

I think the reason is that it makes it clear at the callsite that an argument is being passed "by pointer" and can be mutated. When an argument is passed by reference, you can't see at the callsite if it's passed by reference (can be mutated) or by value (can't be mutated).

Comment 25

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Part 3: Support incoming and outgoing requests/responses

Forwarding r=baku

Comment 26

[Andrea Marchesini [:baku]]

Comment on attachment 8760585 [details] [diff] [review]
Part 2: Enable launching a server in the parent process

Review of attachment 8760585 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for your comments.

::: dom/flyweb/FlyWebPublishedServer.cpp
@@ +89,5 @@
> +                                                        aConnectRequest);
> +  e->Init(this);
> +  e->InitEvent(NS_LITERAL_STRING("websocket"), false, false);
> +
> +  DispatchTrustedEvent(e);

Maybe you can unify FileWebsocketEvent and FireFetchEvent in 1 method only:
FireEvent(InternalREquest* aRequest, const nsAString& aEventName);

::: dom/flyweb/FlyWebPublishedServer.h
@@ +164,5 @@
> +  nsCOMPtr<nsICancelable> mMDNSCancelRegister;
> +  RefPtr<FlyWebPublishedServerParent> mServerParent;
> +};
> +
> +class FlyWebPublishedServerChild : public FlyWebPublishedServer

final ?

@@ +189,5 @@
> +
> +  virtual void ActorDestroy(ActorDestroyReason aWhy) override;
> +
> +private:
> +  virtual ~FlyWebPublishedServerChild() {}

remove virtual if final class

@@ +209,5 @@
> +
> +  virtual bool
> +  Recv__delete__() override;
> +
> +  virtual ~FlyWebPublishedServerParent() {}

remove virtual if final class

::: dom/ipc/ContentParent.cpp
@@ +4162,5 @@
> +bool
> +ContentParent::DeallocPFlyWebPublishedServerParent(PFlyWebPublishedServerParent* aActor)
> +{
> +  auto server = static_cast<FlyWebPublishedServerParent*>(aActor);
> +  NS_RELEASE(server);

RefPtr<FlyWebPublishedServerParent> actor =
  dont_AddRef(static_cast<FlyWebPublishedServerParent*>(aActor));

Comment 27

[Andrea Marchesini [:baku]]

Comment on attachment 8760036 [details] [diff] [review]
Part 6: Add WebSocket support

Review of attachment 8760036 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/flyweb/FlyWebPublishedServer.cpp
@@ +300,5 @@
> +{
> +  LOG_I("FlyWebPublishedServerChild::RecvWebSocketRequest(%p)", this);
> +
> +  RefPtr<InternalRequest> request = new InternalRequest(aRequest);
> +  ErrorResult result;

you are not using it, right?

::: netwerk/ipc/NeckoChild.cpp
@@ +340,5 @@
> +}
> +
> +bool
> +NeckoChild::DeallocPTransportProviderChild(PTransportProviderChild* aActor)
> +{

you should deallocate it here, right?

::: netwerk/protocol/websocket/IPCTransportProvider.cpp
@@ +37,5 @@
> +
> +  return NS_OK;
> +}
> +
> +NS_IMETHODIMP_(mozilla::net::PTransportProviderChild *)

* close to the type

@@ +45,5 @@
> +  return nullptr;
> +}
> +
> +NS_IMETHODIMP
> +TransportProviderParent::OnTransportAvailable(nsISocketTransport *aTransport,

* close to the variable type.

::: netwerk/protocol/websocket/IPCTransportProvider.h
@@ +19,5 @@
> +
> +namespace mozilla {
> +namespace net {
> +
> +class TransportProviderParent : public PTransportProviderParent

final?

@@ +30,5 @@
> +  NS_DECL_ISUPPORTS
> +  NS_DECL_NSITRANSPORTPROVIDER
> +  NS_DECL_NSIHTTPUPGRADELISTENER
> +
> +  void ActorDestroy(ActorDestroyReason aWhy) override {};

you don't have to overwrite it if you don't wanto to use it.

@@ +43,5 @@
> +  nsCOMPtr<nsIAsyncInputStream> mSocketIn;
> +  nsCOMPtr<nsIAsyncOutputStream> mSocketOut;
> +};
> +
> +class TransportProviderChild : public PTransportProviderChild

final?

@@ +52,5 @@
> +
> +  NS_DECL_ISUPPORTS
> +  NS_DECL_NSITRANSPORTPROVIDER
> +
> +  void ActorDestroy(ActorDestroyReason aWhy) override {};

same here.

@@ +58,5 @@
> +private:
> +  ~TransportProviderChild();
> +};
> +
> +}

write here  comments about dom/mozilla

::: netwerk/protocol/websocket/nsITransportProvider.idl
@@ +21,5 @@
>  /**
>   * An interface which can be used to asynchronously request a nsITransport
>   * together with the input and output streams that go together with it.
>   */
> +[uuid(6fcec704-cfd2-46ef-a394-a64d5cb1475c)]

builtinclass ?

Comment 28

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Part 7: Add assertions to all incoming message handlers

Comment 29

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

> ::: netwerk/ipc/NeckoChild.cpp
> @@ +340,5 @@
> > +}
> > +
> > +bool
> > +NeckoChild::DeallocPTransportProviderChild(PTransportProviderChild* aActor)
> > +{
> 
> you should deallocate it here, right?

No, the ownership model for TransportProvider is that the child object is refcounted "normally". I.e. ipdl doesn't hold a strong reference to TransportProviderChild.

When TransportProviderChild goes away, it sends a __delete__ message to the parent.

On the parent side, ipdl holds a strong reference to TransportProviderParent. When the actor is de-alloced it releases the TransportProviderParent.

So effectively the child is "normally" refcounted and entirely owned by whoever holds a strong reference to it. The parent object is owned by the child, but also refcounted and so can outlive the child object.

The only other caveat is that the creation happens from the parent. So to create a TransportProvider, a constructor is sent from the parent to the child. At this time the child gets its first addref.

A reference to the TransportProvider is then sent as part of some other message from the parent to the child. In our case as part of the PFlyWebPublishedServer.WebSocketRequest message. The receiver of that message can then grab the TransportProviderChild and use the refcount that it already has gotten.

I'll add a comment explaining this.

> @@ +30,5 @@
> > +  NS_DECL_ISUPPORTS
> > +  NS_DECL_NSITRANSPORTPROVIDER
> > +  NS_DECL_NSIHTTPUPGRADELISTENER
> > +
> > +  void ActorDestroy(ActorDestroyReason aWhy) override {};
> 
> you don't have to overwrite it if you don't wanto to use it.

Actually you do. In the parent protocol object ActorDestroy is pure virtual. But apparently not in the child, so I'll remove the one in the child.

> @@ +58,5 @@
> > +private:
> > +  ~TransportProviderChild();
> > +};
> > +
> > +}
> 
> write here  comments about dom/mozilla

Not sure what you mean here?

> ::: netwerk/protocol/websocket/nsITransportProvider.idl
> @@ +21,5 @@
> >  /**
> >   * An interface which can be used to asynchronously request a nsITransport
> >   * together with the input and output streams that go together with it.
> >   */
> > +[uuid(6fcec704-cfd2-46ef-a394-a64d5cb1475c)]
> 
> builtinclass ?

builtinclass doesn't do anything for non-scriptable interfaces.

Comment 30

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/555091dfb391
part 1: Remove some properties that are not needed right now. r=djvj
https://hg.mozilla.org/integration/mozilla-inbound/rev/65e49149fbcc
part 2: Enable launching a server in the parent process. r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/5853ea69f6c4
part 3: Support incoming and outgoing requests/responses. r=baku f=bkelly
https://hg.mozilla.org/integration/mozilla-inbound/rev/9be76aad30b1
part 4: Support response body. r=bkelly
https://hg.mozilla.org/integration/mozilla-inbound/rev/754246ebf23b
part 5: Create API on FlyWebPublishedServer for getting back a nsITransport when accepting a websocket request, rather than getting back a WebSocket DOM object. r=djvj
https://hg.mozilla.org/integration/mozilla-inbound/rev/64fb728c174a
part 6: Add e10s support for incoming websocket connections to FlyWebPublishedServer. r=baku,michal
https://hg.mozilla.org/integration/mozilla-inbound/rev/87b9aa4a22e9
part 7: Add assertions checking that the actor is still alive on incoming messages. r=baku

Comment 31

[Carsten Book [:Tomcat]]

sorry had to back this out for bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=29666517&repo=mozilla-inbound

Comment 32

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fce105195b15
Backed out changeset 87b9aa4a22e9 
https://hg.mozilla.org/integration/mozilla-inbound/rev/dad1a1fcd0e5
Backed out changeset 64fb728c174a 
https://hg.mozilla.org/integration/mozilla-inbound/rev/ff0afa409a09
Backed out changeset 754246ebf23b 
https://hg.mozilla.org/integration/mozilla-inbound/rev/29121ecd02eb
Backed out changeset 9be76aad30b1 
https://hg.mozilla.org/integration/mozilla-inbound/rev/53c297b5ceee
Backed out changeset 5853ea69f6c4 
https://hg.mozilla.org/integration/mozilla-inbound/rev/83f917303213
Backed out changeset 65e49149fbcc 
https://hg.mozilla.org/integration/mozilla-inbound/rev/c010252e1bc4
Backed out changeset 555091dfb391 for bustage on a CLOSED TREE

Comment 33

[Carsten Book [:Tomcat]]

seems https://treeherder.mozilla.org/logviewer.html#?job_id=29666521&repo=mozilla-inbound is also related

Comment 34

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Part 2.5: Fix windows compile problems.

Fix windows compile problems.

Sadly it seems like it's not possible to use a single header file to implement both webidl interfaces and ipdl protocols. So we have to split FlyWebPublishedServer.h into two.

Comment 35

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e26bfa28a882
part 1: Remove some properties that are not needed right now. r=djvj
https://hg.mozilla.org/integration/mozilla-inbound/rev/c9cc418f871d
parts 2 and 2.5: Enable launching a server in the parent process. r=baku,ehsan
https://hg.mozilla.org/integration/mozilla-inbound/rev/411ea8234103
part 3: Support incoming and outgoing requests/responses. r=baku f=bkelly
https://hg.mozilla.org/integration/mozilla-inbound/rev/66a0fcfcaee3
part 4: Support response body. r=bkelly
https://hg.mozilla.org/integration/mozilla-inbound/rev/33bbb14d3adc
part 5: Create API on FlyWebPublishedServer for getting back a nsITransport when accepting a websocket request, rather than getting back a WebSocket DOM object. r=djvj
https://hg.mozilla.org/integration/mozilla-inbound/rev/c5ac946a987f
part 6: Add e10s support for incoming websocket connections to FlyWebPublishedServer. r=baku,michal
https://hg.mozilla.org/integration/mozilla-inbound/rev/1b8f7b8f94c4
part 7: Add assertions checking that the actor is still alive on incoming messages. r=baku

Comment 36

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e26bfa28a882
https://hg.mozilla.org/mozilla-central/rev/c9cc418f871d
https://hg.mozilla.org/mozilla-central/rev/411ea8234103
https://hg.mozilla.org/mozilla-central/rev/66a0fcfcaee3
https://hg.mozilla.org/mozilla-central/rev/33bbb14d3adc
https://hg.mozilla.org/mozilla-central/rev/c5ac946a987f
https://hg.mozilla.org/mozilla-central/rev/1b8f7b8f94c4

Comment 37

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 8758631 [details] [diff] [review]
Part 1: Remove some properties that are not needed right now

Approval Request Comment
[Risks and why]: There is very low risk here since this feature is turned off by default. The only way to turn it on is using about:config. So this is very unlikely to affect normal users.

We would like to start getting developer feedback as soon as possible for this feature, which is why we're hoping to ride the 49 train.

[Feature bug #]: This bug (bug 1263991)

[User impact if declined]: Will have to turn off e10s in order to test FlyWeb

[Describe test coverage new/current, TreeHerder]: Only manual testing so far.

[String/UUID change made/needed]: None. There is an interface change made, but it's for a very new interface so extremely unlikely used by addons.

Comment 38

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 8759624 [details] [diff] [review]
Part 4: Support response body

Approval Request Comment
See comment 37

Comment 39

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 8760035 [details] [diff] [review]
Part 5: Add e10s-friendly internal API for accepting websocket connections

Approval Request Comment
See comment 37

Comment 40

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 8760036 [details] [diff] [review]
Part 6: Add WebSocket support

Approval Request Comment
See comment 37

Comment 41

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 8760585 [details] [diff] [review]
Part 2: Enable launching a server in the parent process

Approval Request Comment
See comment 37

Comment 42

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 8760610 [details] [diff] [review]
Part 3: Support incoming and outgoing requests/responses

Approval Request Comment
See comment 37

Comment 43

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 8760653 [details] [diff] [review]
Part 7: Add assertions to all incoming message handlers

Approval Request Comment
See comment 37

Comment 44

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 8760941 [details] [diff] [review]
Part 2.5: Fix windows compile problems.

Approval Request Comment
See comment 37

Comment 45

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8758631 [details] [diff] [review]
Part 1: Remove some properties that are not needed right now

Let's aim this at 49 aurora so developers can test with e10s enabled.

Comment 46

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8760035 [details] [diff] [review]
Part 5: Add e10s-friendly internal API for accepting websocket connections

[Triage Comment]

Comment 47

[Liz Henry (:lizzard) (relman/hg->git project)]

Jonas, if there are any problems landing this on aurora will you be able to handle backout/fixes?

Comment 48

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/c0581820535a
https://hg.mozilla.org/releases/mozilla-aurora/rev/147ddb5ce6f0
https://hg.mozilla.org/releases/mozilla-aurora/rev/b408dc5c1ab8
https://hg.mozilla.org/releases/mozilla-aurora/rev/e9f5c7eb1b28
https://hg.mozilla.org/releases/mozilla-aurora/rev/c4da2d1e3a10
https://hg.mozilla.org/releases/mozilla-aurora/rev/54a3f8e44148
https://hg.mozilla.org/releases/mozilla-aurora/rev/1cf4dc5b5302

Comment 49

[Carsten Book [:Tomcat]]

lizzard, sylvestre: this has (at least part4 has) esr45 approval but it seems that esr approval was never requested, so should this still go to esr45 or was this more a mistake ?

Comment 50

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8759624 [details] [diff] [review]
Part 4: Support response body

It is a typo :)