1272099 - [FlyWeb] Review DOM changes for landing in mozilla-central

Status: RESOLVED FIXED

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

Description

[Kannan Vijayan [:djvj]]

Sub-bug for reviewing DOM changes for landing in m-c.

Comment 1

[Kannan Vijayan [:djvj]]

Attachment: flyweb-dom.patch

Comment 2

[Kannan Vijayan [:djvj]]

Comment on attachment 8751439 [details] [diff] [review]
flyweb-dom.patch

This bug blocks the main review bug for FlyWeb (bug 1272092).  The main review bug has attached all the patches relating to FlyWeb directly, so it can be used to refer to code in other sections.

Comment 3

[u408661]

Since this is for DOM code, let's file it under DOM...

Comment 4

[Andrea Marchesini [:baku]]

Comment on attachment 8751439 [details] [diff] [review]
flyweb-dom.patch

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

Looks good! I just have 1 question about ScriptLoader.cpp and a couple of comments. I give you r- just for these.

::: dom/base/Navigator.cpp
@@ +95,5 @@
>  #include "mozilla/dom/FMRadio.h"
>  #endif
>  
>  #include "nsIDOMGlobalPropertyInitializer.h"
> +#include "mozilla/dom/FlyWebService.h"

The header block in this file is chaotic, but can you put this line somewhere else? Maybe together with some other mozilla/dom/ include?

::: dom/bindings/Bindings.conf
@@ +498,5 @@
>      'wrapperCache': False,
>  },
>  
> +'FlyWebFetchEvent': {
> +    'headerFile': 'FlyWebServerEvents.h',

Remove all the changes in this file. FlyWebServerEsvents.h is included in mozilla/dom. Everything should work auto-magically. If not, tell me why you need this.

::: dom/cache/TypeUtils.cpp
@@ +302,5 @@
>    }
>  
>    nsCOMPtr<nsIInputStream> stream = ReadStream::Create(aIn.body());
> +  uint64_t avail = 0;
> +  stream->Available(&avail);

Available() can fail. You must manage the error value.

::: dom/fetch/Fetch.cpp
@@ +456,5 @@
>      return rv.StealNSResult();
>    }
>  
> +  if (aContentLength) {
> +    impl->GetInternalStream(aStream, rv);

In this way, if aContentLength is 0, aStream is not initialized.
*aStreams = nullptr ?
Or Get the internal stream any way.

@@ +461,5 @@
> +    if (NS_WARN_IF(rv.Failed())) {
> +      return rv.StealNSResult();
> +    }
> +  }
> +  

extra spaces.

@@ +518,3 @@
>  
> +  return aContentLength ?
> +    NS_NewCStringInputStream(aStream, encoded) : NS_OK;

here as well, aStream should be initialized somehow.
Set it to null or call NS_NewCStringInputStream() also if aContentLength is 0.

More important: this change should be documented.

@@ +577,3 @@
>  {
>    MOZ_ASSERT(aStream);
> +  MOZ_ASSERT(!*aStream);

ah! Well, ok. I prefer *aStream = nullptr;

::: dom/fetch/Fetch.h
@@ +45,4 @@
>  /*
>   * Creates an nsIInputStream based on the fetch specifications 'extract a byte
>   * stream algorithm' - http://fetch.spec.whatwg.org/#concept-bodyinit-extract.
>   * Stores content type in out param aContentType.

Write here a comment saying that aStream will be nullptr if aContentLength is 0.

::: dom/fetch/FetchDriver.cpp
@@ +519,5 @@
>        NS_WARNING("Failed to visit all headers.");
>      }
> +
> +    // Http-channels lie about their content-length sometimes
> +    ErrorResult res;

result;

@@ +523,5 @@
> +    ErrorResult res;
> +    if (response->Headers()->Has(NS_LITERAL_CSTRING("content-encoding"), res) ||
> +        response->Headers()->Has(NS_LITERAL_CSTRING("transfer-encoding"), res)) {
> +      contentLength = 0;
> +    }

This is wrong. You must manage the error result of ErrorResult. You should do:

if (res.Failed()) {
  res.SupressException();
}

Check the DTOR of ErrorResult.

::: dom/fetch/InternalResponse.h
@@ +282,5 @@
>    const uint16_t mStatus;
>    const nsCString mStatusText;
>    RefPtr<InternalHeaders> mHeaders;
>    nsCOMPtr<nsIInputStream> mBody;
> +  uint64_t mBodySize;

This is not initialized in the CTOR.

::: dom/fetch/Response.cpp
@@ +211,5 @@
> +    aRv = ExtractByteStreamFromBody(aBody.Value(),
> +                                    getter_AddRefs(bodyStream),
> +                                    contentType,
> +                                    bodySize);
> +    internalResponse->SetBody(bodyStream, bodySize);

Ok this is an existing error... Can you please do:

if (NS_WARN_IF(aRv.Failed())) {
  return nullptr;
}

an ErrorResult cannot change the error value.

::: dom/webidl/moz.build
@@ +159,5 @@
>      'FileList.webidl',
>      'FileMode.webidl',
>      'FileReader.webidl',
>      'FileReaderSync.webidl',
> +    #'FlyWebConnection.webidl',

Don't do that. If this is not needed, add it in a separate patch.

@@ +786,5 @@
>      'DeviceStorageChangeEvent.webidl',
>      'DOMTransactionEvent.webidl',
>      'DownloadEvent.webidl',
>      'ErrorEvent.webidl',
> +    #'FlyWebClientConnectEvent.webidl',

here as well.

::: dom/workers/ScriptLoader.cpp
@@ +656,5 @@
>  
>      // We synthesize the result code, but its never exposed to content.
>      RefPtr<mozilla::dom::InternalResponse> ir =
>        new mozilla::dom::InternalResponse(200, NS_LITERAL_CSTRING("OK"));
> +    ir->SetBody(loadInfo.mCacheReadStream, 0);

This is super confusing... Why do you think it's OK to pass 0 as stream length?

::: dom/flyweb/FlyWebServerEvents.cpp
@@ +7,5 @@
> +#include "mozilla/dom/EventBinding.h"
> +#include "mozilla/dom/FlyWebServerEvents.h"
> +#include "mozilla/dom/FlyWebFetchEventBinding.h"
> +#include "mozilla/dom/FlyWebWebSocketEventBinding.h"
> +#include "js/GCAPI.h"

alphabetic order?

@@ +9,5 @@
> +#include "mozilla/dom/FlyWebFetchEventBinding.h"
> +#include "mozilla/dom/FlyWebWebSocketEventBinding.h"
> +#include "js/GCAPI.h"
> +#include "mozilla/dom/Nullable.h"
> +#include "mozilla/dom/FlyWebPublishedServer.h"

here as well.

@@ +44,5 @@
> +  , mRequest(aRequest)
> +  , mInternalRequest(aInternalRequest)
> +  , mServer(aServer)
> +  , mResponded(false)
> +{

MOZ_ASSERT(aServer);
MOZ_ASSERT(aRequesT);
MOZ_ASSERT(aInternalRequest);

@@ +132,5 @@
> +  mServer->OnWebSocketResponse(mInternalRequest, aResponse);
> +}
> +
> +
> +

extra lines. Usually 1 or 2. Not 3 :)

::: dom/flyweb/FlyWebServerEvents.h
@@ +45,5 @@
> +  }
> +
> +  void RespondWith(Promise& aArg, ErrorResult& aRv);
> +
> +  explicit FlyWebFetchEvent(FlyWebPublishedServer* aServer,

no explicit when the CTOR has more then 1 param.

@@ +64,5 @@
> +
> +class FlyWebWebSocketEvent final : public FlyWebFetchEvent
> +{
> +public:
> +  explicit FlyWebWebSocketEvent(FlyWebPublishedServer* aServer,

no explicit when the CTOR has more then 1 param.

@@ +76,5 @@
> +  already_AddRefed<WebSocket> Accept(const Optional<nsAString>& aProtocol,
> +                                     ErrorResult& aRv);
> +
> +private:
> +  virtual ~FlyWebWebSocketEvent() {};

no virtual DTOR when final.

::: dom/webidl/FlyWebPublish.webidl
@@ +2,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/.
> + */
> +

Do we have any doucmentation about this webIDL? If yes, can you add here the URL?

@@ +4,5 @@
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/.
> + */
> +
> +interface FlyWebPublishedServer : EventTarget {
> +  //readonly attribute DOMString id;

Why this is comment out? Add a comment or remove it. Or add the bug ID of the follow up.

@@ +10,5 @@
> +  readonly attribute DOMString? category;
> +  readonly attribute boolean http;
> +  readonly attribute boolean message;
> +  readonly attribute DOMString? uiUrl;
> +  //readonly attribute deep_frozen object? data;

ditto.

@@ +12,5 @@
> +  readonly attribute boolean message;
> +  readonly attribute DOMString? uiUrl;
> +  //readonly attribute deep_frozen object? data;
> +
> +  // start(); // to make sure 'connect' events are not fired before a listener is registered

ditto.

@@ +38,5 @@
> +  // DOMString id;
> +  // ArrayBuffer? cert; // null if not encrypted
> +  // int port;
> +  // DOMString deviceName;
> +  // DOMString origin;

all of these?

::: dom/webidl/Navigator.webidl
@@ +144,5 @@
> +  Promise<FlyWebPublishedServer> publishServer(DOMString name,
> +                                               optional FlyWebPublishOptions options);
> +
> +  //[NewObject, Pref="dom.flyweb.enabled"]
> +  //Promise<FlyWebConnection> connectToServer(optional FlyWebFilter filter);

follow up? Remove or write a comment/bug ID.

Comment 5

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

> @@ +523,5 @@
> > +    ErrorResult res;
> > +    if (response->Headers()->Has(NS_LITERAL_CSTRING("content-encoding"), res) ||
> > +        response->Headers()->Has(NS_LITERAL_CSTRING("transfer-encoding"), res)) {
> > +      contentLength = 0;
> > +    }
> 
> This is wrong. You must manage the error result of ErrorResult. You should
> do:
> 
> if (res.Failed()) {
>   res.SupressException();
> }
> 
> Check the DTOR of ErrorResult.

There's a bunch of methods on InternalResponse/InternalHeaders/InternalRequest which take ErrorResults, but which we never expect should fail. In particular, InternalHeaders::Has is supposed to throw if the passed in header name is not a valid name, which should never be the case here.

My hope was to simply assert in debug builds if these functions throw. It looked like ErrorResult will throw if it has an unhandled error when its dtor runs, which would accomplish that goal.

Is that ok? Or do you have another suggested approach?

Comment 6

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

> ::: dom/bindings/Bindings.conf
> @@ +498,5 @@
> >      'wrapperCache': False,
> >  },
> >  
> > +'FlyWebFetchEvent': {
> > +    'headerFile': 'FlyWebServerEvents.h',
> 
> Remove all the changes in this file. FlyWebServerEsvents.h is included in
> mozilla/dom. Everything should work auto-magically. If not, tell me why you
> need this.

The documentation for webidl generated events said to include this if the header name isn't exactly <event-interface-name>.h

> ::: dom/cache/TypeUtils.cpp
> @@ +302,5 @@
> >    }
> >  
> >    nsCOMPtr<nsIInputStream> stream = ReadStream::Create(aIn.body());
> > +  uint64_t avail = 0;
> > +  stream->Available(&avail);
> 
> Available() can fail. You must manage the error value.

If the function fails, I think we simply want to set 'avail' to 0, which already happens here. Maybe a comment is enough?

> ::: dom/fetch/Fetch.cpp
> @@ +456,5 @@
> >      return rv.StealNSResult();
> >    }
> >  
> > +  if (aContentLength) {
> > +    impl->GetInternalStream(aStream, rv);
> 
> In this way, if aContentLength is 0, aStream is not initialized.
> *aStreams = nullptr ?
> Or Get the internal stream any way.

Gecko calling convention is that addreffed out params should always be initialized to nullptr before the function is called (which is required for getter_AddRefs to work, and is also done by getter_AddRefs). So should be ok to leave aStream as nullptr?


> ::: dom/workers/ScriptLoader.cpp
> @@ +656,5 @@
> >  
> >      // We synthesize the result code, but its never exposed to content.
> >      RefPtr<mozilla::dom::InternalResponse> ir =
> >        new mozilla::dom::InternalResponse(200, NS_LITERAL_CSTRING("OK"));
> > +    ir->SetBody(loadInfo.mCacheReadStream, 0);
> 
> This is super confusing... Why do you think it's OK to pass 0 as stream
> length?

0 means "we don't know the size of the stream".

It's needed when an InternalResult is created using for example fetch("someuri.html"). In that case the server might stream the response body, and we won't know the size at the time when the InternalResult object is created and processed.

This should be documented though.

It would be great if we could know the size of the object in the cache though. But it didn't seem easy to do that, and isn't always possible since I think you can store an object in the cache before we have downloaded it fully.

So seems fine to use 0 always for now. Knowing the exact size is mainly a performance optimization, so we can improve this later if needed.


> @@ +10,5 @@
> > +  readonly attribute DOMString? category;
> > +  readonly attribute boolean http;
> > +  readonly attribute boolean message;
> > +  readonly attribute DOMString? uiUrl;
> > +  //readonly attribute deep_frozen object? data;
> 
> ditto.

I think we can remove this one for now.

> @@ +12,5 @@
> > +  readonly attribute boolean message;
> > +  readonly attribute DOMString? uiUrl;
> > +  //readonly attribute deep_frozen object? data;
> > +
> > +  // start(); // to make sure 'connect' events are not fired before a listener is registered
> 
> ditto.

Oh. We should actually implement the start() function. I had forgotten about that.

This is modeled after the MessagePort interface, it's not a great design, but it's the best we can do until with the current infrastructure that the DOM has.

The problem is that we might have incoming HTTP requests to the server before the webpage is able to register a handler for the "fetch" event. This is especially true since the FlyWebBublishedServer object is delivered through Promises, which means that several asynchronous steps might happen before the server object reaches the code which registers the event handler.

Because of that, the start() function exists so that the webpage can indicate that it now has registered all critical event handlers and that it's ok to start firing "fetch" events.

Additionally, the onfetch and onwebsocket property setters should call start().

Until start() is called, the server object should queue all events and then fire them asynchronously after start() has been called.

This is how the MessagePort interface also works.

We should probably fix this in a followup bug rather than in this bug.

> ::: dom/webidl/Navigator.webidl
> @@ +144,5 @@
> > +  Promise<FlyWebPublishedServer> publishServer(DOMString name,
> > +                                               optional FlyWebPublishOptions options);
> > +
> > +  //[NewObject, Pref="dom.flyweb.enabled"]
> > +  //Promise<FlyWebConnection> connectToServer(optional FlyWebFilter filter);
> 
> follow up? Remove or write a comment/bug ID.

Yeah, all references to connection objects should be removed from this patch for now. It's something we should implement later.

Comment 7

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

I pushed a patch to larch which gets rid of FlyWebConnection and FlyWebPublishedServer.port

Comment 8

[Andrea Marchesini [:baku]]

> > > +'FlyWebFetchEvent': {
> > > +    'headerFile': 'FlyWebServerEvents.h',

> The documentation for webidl generated events said to include this if the
> header name isn't exactly <event-interface-name>.h

Right. So it must be mozilla/dom/FlyWebServerEvents.h.
Thanks for your comment.

Comment 9

[Kannan Vijayan [:djvj]]

Thanks for the quick review baku!  I'm handling the review responses.. so here goes.

If I didn't respond to a comment in this post, it means I did what you suggested.  The only places where I wrote a response is where there is something extra to convey or clarify.


> Remove all the changes in this file. FlyWebServerEsvents.h is included in mozilla/dom. Everything should work auto-magically. If not, tell me why you need this.

As Jonas noted, these entries in Bindings.conf are needed because the webidl interface name doesn't match with the file name.  The build doesn't work without them.

> > +  stream->Available(&avail);
> Available() can fail. You must manage the error value.

Fixed with:

  uint64_t avail;
  nsresult rv = stream->Available(&avail);

  int bodySize = NS_SUCCEEDED(rv) ? avail : -1;
  ir->SetBody(stream, bodySize);


> ::: dom/fetch/Fetch.cpp
> @@ +456,5 @@
> >      return rv.StealNSResult();
> >    }
> >
> > +  if (aContentLength) {
> > +    impl->GetInternalStream(aStream, rv);
>
> In this way, if aContentLength is 0, aStream is not initialized.
> *aStreams = nullptr ?
> Or Get the internal stream any way.

Changed to always get the internal stream even if content length is 0.
We instead use -1 elsewhere to represent unknown-length streams.

> ::: dom/fetch/Fetch.h
> @@ +45,4 @@
> >  /*
> >   * Creates an nsIInputStream based on the fetch specifications 'extract a byte
> >   * stream algorithm' - http://fetch.spec.whatwg.org/#concept-bodyinit-extract.
> >   * Stores content type in out param aContentType.
>
> Write here a comment saying that aStream will be nullptr if aContentLength is 0.

No need for this anymore since we're always initializing aStream, even if contentLength is 0.

> ::: dom/fetch/FetchDriver.cpp
> @@ +519,5 @@
> >        NS_WARNING("Failed to visit all headers.");
> >      }
> > +
> > +    // Http-channels lie about their content-length sometimes
> > +    ErrorResult res;
>
> result;

Done.

> @@ +523,5 @@
> > +    ErrorResult res;
> > +    if (response->Headers()->Has(NS_LITERAL_CSTRING("content-encoding"), res) ||
> > +        response->Headers()->Has(NS_LITERAL_CSTRING("transfer-encoding"), res)) {
> > +      contentLength = 0;
> > +    }
>
> This is wrong. You must manage the error result of ErrorResult. You should do:
>
> if (res.Failed()) {
>   res.SupressException();
> }
>
> Check the DTOR of ErrorResult.

This should never fail, so instead I changed to MOZ_ASSERT(!res.Failed()) here.

> ::: dom/fetch/InternalResponse.h
> @@ +282,5 @@
> >    const uint16_t mStatus;
> >    const nsCString mStatusText;
> >    RefPtr<InternalHeaders> mHeaders;
> >    nsCOMPtr<nsIInputStream> mBody;
> > +  uint64_t mBodySize;
>
> This is not initialized in the CTOR.

Done.  mBodySize is now a int64_t and gets initialized to -1.

> ::: dom/workers/ScriptLoader.cpp
> @@ +656,5 @@
> >
> >      // We synthesize the result code, but its never exposed to content.
> >      RefPtr<mozilla::dom::InternalResponse> ir =
> >        new mozilla::dom::InternalResponse(200, NS_LITERAL_CSTRING("OK"));
> > +    ir->SetBody(loadInfo.mCacheReadStream, 0);
>
> This is super confusing... Why do you think it's OK to pass 0 as stream length?

Fixed to pass -1 now, indicating unknown size.


> ::: dom/webidl/FlyWebPublish.webidl
> @@ +2,5 @@
> > +/* This Source Code Form is subject to the terms of the Mozilla Public
> > + * License, v. 2.0. If a copy of the MPL was not distributed with this
> > + * file, You can obtain one at http://mozilla.org/MPL/2.0/.
> > + */
> > +
>
> Do we have any doucmentation about this webIDL? If yes, can you add here the URL?

We don't really have a up-to-date spec to reference yet.

Comment 10

[Kannan Vijayan [:djvj]]

Attachment: flyweb-dom-updates.patch

Update delta patch for the dom changeset.

Comment 11

[Kannan Vijayan [:djvj]]

Landed the first set of changes in response to review:
http://hg.mozilla.org/projects/larch/rev/a08929a7e343

Comment 12

[Kannan Vijayan [:djvj]]

Attachment: flyweb-dom.patch

Updated "full diff" of DOM changes between moz-cental and flyweb branches.

Comment 13

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

The certOverrideService changes made their way into the updated patch. Not sure if we should do those in a separate bug. At the very least we should have David Keeler review those.

Comment 14

[Andrea Marchesini [:baku]]

Comment on attachment 8754504 [details] [diff] [review]
flyweb-dom-updates.patch

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

::: dom/fetch/Response.cpp
@@ +212,5 @@
>                                      getter_AddRefs(bodyStream),
>                                      contentType,
>                                      bodySize);
>      internalResponse->SetBody(bodyStream, bodySize);
> +    if (NS_WARN_IF(aRv.Failed())) {

this should happen before calling SetBody... right?

Comment 15

[Andrea Marchesini [:baku]]

Comment on attachment 8754921 [details] [diff] [review]
flyweb-dom.patch

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

Somebody else should review the security code changes.

::: security/manager/ssl/nsCertOverrideService.cpp
@@ +437,5 @@
> +    AddEntryToList(aHostName, aPort,
> +                   nullptr, // No cert to keep alive
> +                   true, // temporary 
> +                   mDottedOidForStoringNewHashes,
> +                   aCertFingerprint, 

extra spaces here.

::: security/manager/ssl/nsICertOverrideService.idl
@@ +61,5 @@
> +   *  Host:Port is a primary key, only one entry per host:port can exist.
> +   *  The implementation will decide which fingerprint alg is used.
> +   *
> +   *  @param aHostName The host (punycode) this mapping belongs to
> +   *  @param aPort The port this mapping belongs to, if it is -1 then it 

extra space here.

Comment 16

[Kannan Vijayan [:djvj]]

Pushed fixes relating to review comments: https://hg.mozilla.org/projects/larch/rev/6a169cc1adef

Comment 17

[Kannan Vijayan [:djvj]]

Attachment: flyweb-dom.patch

Updated amalgamated dom-changes patch.  Forwarding r+ from previous.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/576019c74103
Bug 1272101 - FlyWeb core implementation, DOM and Network changes. r=baku r=hurley

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/576019c74103