The Fetch specification actually defines all DOM facing resource fetching, but this bug only deals with calls via fetch() and Fetch related APIs exposed to ServiceWorkers. The underlying FetchDriver implementation will also be used by the Cache/FetchStorage API internally.

Kyle, This is a basic outline of the FetchDriver for the main thread. It can do a simple fetch() with no security checks (except those provided by Necko). The meat is FetchDriver.cpp (implements internals) and Fetch.cpp (DOM exposed fetch() and mapping it to internals). Does this look reasonable?

Also, what is a good way to represent the `context`. I see WorkerPrivate::LoadInfo maintains a nsIPrincipal, nsIScriptContext and a nsPIDOMWindow.

Comment on attachment 8495553 [details] [diff] [review] Request implementation Review of attachment 8495553 [details] [diff] [review]: ----------------------------------------------------------------- I would like to read it again. Thanks! ::: dom/bindings/Bindings.conf @@ +870,5 @@ > 'nativeType': 'nsDOMCSSRect', > }, > > +'Request': { > + 'binaryNames': { 'headers': 'Headers_' }, the value should be 'headers_' and not 'Headers_' ::: dom/fetch/Fetch.cpp @@ +19,5 @@ > +nsresult > +ExtractByteStreamFromBody(const OwningArrayBufferOrArrayBufferViewOrScalarValueStringOrURLSearchParams& aBodyInit, > + nsIInputStream** aStream, > + nsCString& aContentType) > +{ MOZ_ASSERT aStream and the correct thread. @@ +45,5 @@ > + } > + } else if (aBodyInit.IsScalarValueString()) { > + nsString str = aBodyInit.GetAsScalarValueString(); > + > + nsCOMPtr<nsIUnicodeEncoder> encoder = EncodingUtils::EncoderForEncoding("UTF-8"); check if this is null. @@ +63,5 @@ > + rv = encoder->Convert(str.get(), &srcLen, destBuffer, &destBufferLen); > + if (NS_WARN_IF(NS_FAILED(rv))) { > + return rv; > + } > + it can happen that the converted string is smaller. In this case I think you should do: int32_t outputLen = destBufferLen; rv = encoder->Convert(str.get(), &srcLen, destBuffer, &outputLen); if (NS_WARN_IF(NS_FAILED(rv))) { ... if (outputLen < destBufferLen) { encoded.Truncate(outputLen); } @@ +70,5 @@ > + if (NS_WARN_IF(NS_FAILED(rv))) { > + return rv; > + } > + > + aContentType = NS_LITERAL_CSTRING("text/plain;charset=UTF-8"); you don't set acontentType if we have an ArrayBuffer. Is it ok? ::: dom/fetch/Headers.cpp @@ +98,5 @@ > + } else if (aInit.IsByteStringMozMap()) { > + headers->Fill(aInit.GetAsByteStringMozMap(), aRv); > + } > + > + if (aRv.Failed()) { NS_WARN_IF ? @@ +111,5 @@ > + , mGuard(aOther.mGuard) > +{ > + SetIsDOMBinding(); > + ErrorResult result; > + Fill(aOther, result); If this cannot fail, do a MOZ_ASSERT or write NS_WARNING("Failed to..."); The real question is: can it fail? and if yes, what do we do? ::: dom/fetch/Headers.h @@ +85,5 @@ > > virtual JSObject* WrapObject(JSContext* aCx); > nsISupports* GetParentObject() const { return mOwner; } > > + void SwappedFill(Headers* aHeaders, ErrorResult&); If you don't use this ErrorResult, remove it. ::: dom/fetch/InternalRequest.cpp @@ +29,5 @@ > + > + if (NS_IsMainThread()) { > + nsIPrincipal* principal = aGlobal->PrincipalOrNull(); > + MOZ_ASSERT(principal); > + nsContentUtils::GetASCIIOrigin(principal, copy->mOrigin); This can fail. ::: dom/fetch/InternalRequest.h @@ +236,5 @@ > + nsCString mURL; > + nsRefPtr<Headers> mHeaders; > + nsCOMPtr<nsIInputStream> mBodyStream; > + > + nsIGlobalObject* mClient; Should it be a nsCOMPtr? If not write a comment about what keeps this alive. ::: dom/fetch/Request.cpp @@ +27,4 @@ > > NS_IMPL_CYCLE_COLLECTING_ADDREF(Request) > NS_IMPL_CYCLE_COLLECTING_RELEASE(Request) > +NS_IMPL_CYCLE_COLLECTION_WRAPPERCACHE(Request, mOwner) why no mHeaders? @@ +88,5 @@ > + nsCOMPtr<nsPIDOMWindow> window = do_QueryInterface(global); > + MOZ_ASSERT(window); > + nsCOMPtr<nsIURI> docURI = window->GetDocumentURI(); > + nsCString spec; > + docURI->GetSpec(spec); this can fail. @@ +93,5 @@ > + nsRefPtr<mozilla::dom::URL> url = > + mozilla::dom::URL::Constructor(aGlobal, input, > + NS_ConvertUTF8toUTF16(spec), aRv); > + if (aRv.Failed()) { > + aRv.ThrowTypeError(MSG_URL_PARSE_ERROR); So, actually you should return the correct aRv value. The constructor can fail for a couple of reasons. Then, why |aRv.ThrowTypeError(MSG_INVALID_URL, &label);| is not enough? @@ +99,5 @@ > + } > + > + url->Stringify(sURL, aRv); > + if (aRv.Failed()) { > + aRv.ThrowTypeError(MSG_URL_PARSE_ERROR); Same issue here. @@ +111,5 @@ > + nsString baseURL = NS_ConvertUTF8toUTF16(worker->GetLocationInfo().mHref); > + nsRefPtr<workers::URL> url = > + workers::URL::Constructor(aGlobal, input, baseURL, aRv); > + if (aRv.Failed()) { > + return nullptr; here you use aRv and not MSG_URL_PARSER_ERROR. @@ +116,5 @@ > + } > + > + url->Stringify(sURL, aRv); > + if (aRv.Failed()) { > + return nullptr; here again. @@ +161,5 @@ > + > + // Step 13 - copy of domRequest headers. > + nsRefPtr<Headers> headers = new Headers(*domRequestHeaders); > + > + if (aInit.mHeaders.WasPassed()) { can you change this so that you do: if (aInit.mHedaers.WasPassed()) { ... } else { headers = new Headers(*domREquestHeaders); } ? @@ +187,5 @@ > + return nullptr; > + } > + } > + > + domRequestHeaders->SwappedFill(headers, aRv); this aRv is not needed. @@ +197,5 @@ > + const OwningArrayBufferOrArrayBufferViewOrScalarValueStringOrURLSearchParams& bodyInit = aInit.mBody.Value(); > + nsCOMPtr<nsIInputStream> stream; > + nsCString contentType; > + aRv = ExtractByteStreamFromBody(bodyInit, > + getter_AddRefs(stream), contentType); you should check aRv @@ +244,5 @@ > +nsresult > +DecodeUTF8(const nsCString& aBuffer, nsString& aDecoded) > +{ > + nsCOMPtr<nsIUnicodeDecoder> decoder = > + EncodingUtils::DecoderForEncoding("UTF-8"); can this be null? @@ +262,5 @@ > + int32_t srcLen = (int32_t) aBuffer.Length(); > + rv = decoder->Convert(aBuffer.get(), &srcLen, destBuffer, &destBufferLen); > + if (NS_WARN_IF(NS_FAILED(rv))) { > + return rv; > + } same check of the output size. @@ +264,5 @@ > + if (NS_WARN_IF(NS_FAILED(rv))) { > + return rv; > + } > + > + aDecoded.SetLength(destBufferLen); SetLength can fail. Use Truncate()

(In reply to Andrea Marchesini (:baku) from comment #4) > Comment on attachment 8495553 [details] [diff] [review] > Request implementation > > > ::: dom/fetch/Fetch.cpp > @@ +19,5 @@ > > +nsresult > > +ExtractByteStreamFromBody(const OwningArrayBufferOrArrayBufferViewOrScalarValueStringOrURLSearchParams& aBodyInit, > > + nsIInputStream** aStream, > > + nsCString& aContentType) > > +{ > > MOZ_ASSERT aStream and the correct thread. This function is thread safe, not sure why I need to assert any correct thread. > > @@ +63,5 @@ > > + rv = encoder->Convert(str.get(), &srcLen, destBuffer, &destBufferLen); > > + if (NS_WARN_IF(NS_FAILED(rv))) { > > + return rv; > > + } > > + > > it can happen that the converted string is smaller. In this case I think you > should do: > > int32_t outputLen = destBufferLen; > rv = encoder->Convert(str.get(), &srcLen, destBuffer, &outputLen); > if (NS_WARN_IF(NS_FAILED(rv))) { ... > > if (outputLen < destBufferLen) { > encoded.Truncate(outputLen); > } I am doing just that right now since destBufferLen will have the value of the actual length. And Truncate calls SetLength internally (of course verifying that the new length is <= old length and so not causing failure), but the encoder documentation seems to imply that what it writes out to outputLen will be <= destBufferLen, so I think using SetLength is fine. > > @@ +70,5 @@ > > + if (NS_WARN_IF(NS_FAILED(rv))) { > > + return rv; > > + } > > + > > + aContentType = NS_LITERAL_CSTRING("text/plain;charset=UTF-8"); > > you don't set acontentType if we have an ArrayBuffer. Is it ok? The content type is used to send the HTTP Header. Reading out the body on the client, an empty content type is fine. > > ::: dom/fetch/InternalRequest.h > @@ +236,5 @@ > > + nsCString mURL; > > + nsRefPtr<Headers> mHeaders; > > + nsCOMPtr<nsIInputStream> mBodyStream; > > + > > + nsIGlobalObject* mClient; > > Should it be a nsCOMPtr? If not write a comment about what keeps this alive. Hmm... I'll have to think about this. > > ::: dom/fetch/Request.cpp > @@ +27,4 @@ > > > > NS_IMPL_CYCLE_COLLECTING_ADDREF(Request) > > NS_IMPL_CYCLE_COLLECTING_RELEASE(Request) > > +NS_IMPL_CYCLE_COLLECTION_WRAPPERCACHE(Request, mOwner) > > why no mHeaders? Because it doesn't exist anymore. Should I be CCing the internal request? > > @@ +264,5 @@ > > + if (NS_WARN_IF(NS_FAILED(rv))) { > > + return rv; > > + } > > + > > + aDecoded.SetLength(destBufferLen); > > SetLength can fail. Use Truncate() same here.

Requested changes. For now I have removed nsIGlobalObject* mClient from InternalRequest. It is only used in the actual fetch algorithm, to determine whether the request is coming from a ServiceWorker or not. I will add it later if required, although we have easier, implementation specific ways to check that.

Guard enforcement is required whenever we deal with headers. See Bug 1074286.

Response can be backed by a network request or cache read, so its consume body algorithms should actually support streams that aren't instantly available when the promise is requested by content. This patch includes refactoring to have the consume body algorithm shared amongst Response and Request (using CRTP). I will add support for making a Promise wait on the stream being ready in a separate patch.

Comment on attachment 8496326 [details] [diff] [review] Patch 1: Request implementation Review of attachment 8496326 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/fetch/Fetch.cpp @@ +27,5 @@ > + nsCOMPtr<nsIInputStream> byteStream; > + if (aBodyInit.IsArrayBuffer()) { > + const ArrayBuffer& buf = aBodyInit.GetAsArrayBuffer(); > + buf.ComputeLengthAndData(); > + //XXXnsm reinterpret_cast<> is used in DOMParser, should be ok. This comment is for a follow up or is just a normal comment? ::: dom/fetch/Headers.cpp @@ +101,1 @@ > if (aRv.Failed()) { NS_WARN_IF @@ +335,5 @@ > } > > void > +Headers::SwappedFill(Headers* aHeaders) > +{ MOZ_ASSERT(aHeaders); ::: dom/fetch/InternalRequest.cpp @@ +40,5 @@ > + workers::WorkerPrivate* worker = workers::GetCurrentThreadWorkerPrivate(); > + MOZ_ASSERT(worker); > + worker->AssertIsOnWorkerThread(); > + > + workers::WorkerPrivate::LocationInfo location = worker->GetLocationInfo(); workers::WorkerPrivate::LocationInfo& ::: dom/fetch/InternalRequest.h @@ +9,5 @@ > +#include "mozilla/dom/RequestBinding.h" > +#include "mozilla/dom/UnionTypes.h" > + > +#include "nsIContentPolicy.h" > +#include "nsIInputStream.h" forward declarations of this 2. @@ +12,5 @@ > +#include "nsIContentPolicy.h" > +#include "nsIInputStream.h" > +#include "nsISupportsImpl.h" > + > +#include "Headers.h" mozilla/dom/Headers.h ::: dom/fetch/Request.cpp @@ +88,5 @@ > + if (aInput.IsScalarValueString()) { > + nsString input; > + input.Assign(aInput.GetAsScalarValueString()); > + > + nsString sURL; sURL seems 'static' URL. maybe a different name. @@ +102,5 @@ > + > + nsRefPtr<mozilla::dom::URL> url = > + mozilla::dom::URL::Constructor(aGlobal, input, > + NS_ConvertUTF8toUTF16(spec), aRv); > + if (aRv.Failed()) { dom::URL should be enough. @@ +146,5 @@ > + request->SetCredentialsMode(credentials); > + } > + > + if (aInit.mMethod.WasPassed()) { > + nsCString method = aInit.mMethod.Value(); what about something like: nsCString method = aInit.mMethod.value(); ToLowerCase(method); and then: method.EqualsLiteral("options") ... ? just to void NS_ToLower() several times for the same string. @@ +166,5 @@ > + > + nsRefPtr<Request> domRequest = new Request(global, request); > + nsRefPtr<Headers> domRequestHeaders = domRequest->Headers_(); > + > + // Step 13 - copy of domRequest headers. We don't have comments for the other steps :) @@ +182,5 @@ > + > + if (domRequest->Mode() == RequestMode::No_cors) { > + nsCString method; > + domRequest->GetMethod(method); > + if (!method.LowerCaseEqualsLiteral("get") && same issue here. ::: dom/fetch/Request.h @@ +14,4 @@ > #include "mozilla/dom/RequestBinding.h" > #include "mozilla/dom/UnionTypes.h" > > +#include "InternalRequest.h" mozilla/dom/InternalRequest.h

Comment on attachment 8496990 [details] [diff] [review] Patch 1: interdiff to use Headers::Fill() Review of attachment 8496990 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/fetch/Headers.h @@ -87,5 @@ > > virtual JSObject* WrapObject(JSContext* aCx); > nsISupports* GetParentObject() const { return mOwner; } > > - void SwappedFill(Headers* aHeaders); There is something missing in this patch. SwappedFill has been removed, but I don't see any change in Headers.cpp ::: dom/fetch/Request.cpp @@ +201,1 @@ > if (aRv.Failed()) { NS_WARN_IF

Comment on attachment 8497172 [details] [diff] [review] Patch 2: Response implementation Review of attachment 8497172 [details] [diff] [review]: ----------------------------------------------------------------- Ask bz this CC issue. For the rest it seems ok to me. ::: dom/fetch/Fetch.cpp @@ +93,5 @@ > +ExtractFromURLSearchParams(const URLSearchParams& aParams, > + nsIInputStream** aStream, > + nsCString& aContentType) > +{ > + nsString serialized; nsAutoString @@ +117,2 @@ > } else if (aBodyInit.IsScalarValueString()) { > + nsString str; nsAutoString @@ +140,5 @@ > + } else if (aBodyInit.IsArrayBufferView()) { > + const ArrayBufferView& buf = aBodyInit.GetAsArrayBufferView(); > + return ExtractFromArrayBufferView(buf, aStream); > + } else if (aBodyInit.IsScalarValueString()) { > + nsString str; nsAutoString @@ +212,5 @@ > + > + if (!stream) { > + aRv = NS_NewByteInputStream(getter_AddRefs(stream), "", 0, > + NS_ASSIGNMENT_COPY); > + if (aRv.Failed()) { NS_WARN_IF @@ +218,5 @@ > + } > + } > + > + AutoJSAPI api; > + api.Init(DerivedClass()->GetParentObject()); are we sure this GetParentObject() is always a good value? Can you put an assert somewhere just to check this? @@ +232,5 @@ > + return nullptr; > + } > + > + aRv = NS_ReadInputStreamToString(stream, buffer, len); > + if (aRv.Failed()) { NS_WARN_IF @@ +264,5 @@ > + memcpy(blobData, buffer.BeginReading(), blobLen); > + blob = DOMFile::CreateMemoryFile(blobData, blobLen, > + NS_ConvertUTF8toUTF16(mMimeType)); > + } else { > + aRv = NS_ERROR_OUT_OF_MEMORY; aRv.Throw(NS_ERROR_OUT_OF_MEMORY); @@ +271,5 @@ > + > + JS::Rooted<JS::Value> jsBlob(cx); > + if (NS_IsMainThread()) { > + aRv = nsContentUtils::WrapNative(cx, blob, &jsBlob); > + if (aRv.Failed()) { NS_WARN_IF @@ +281,5 @@ > + promise->MaybeResolve(cx, jsBlob); > + return promise.forget(); > + } > + case CONSUME_JSON: { > + nsString decoded; nsAutoString @@ +283,5 @@ > + } > + case CONSUME_JSON: { > + nsString decoded; > + aRv = DecodeUTF8(buffer, decoded); > + if (aRv.Failed()) { NS_WARN_IF (and elsewhere) @@ +299,5 @@ > + promise->MaybeResolve(cx, json); > + return promise.forget(); > + } > + case CONSUME_TEXT: { > + nsString decoded; nsAutoString ::: dom/fetch/InternalResponse.h @@ +26,5 @@ > + > + static already_AddRefed<InternalResponse> > + NetworkError() > + { > + nsRefPtr<InternalResponse> response = new InternalResponse(0, NS_LITERAL_CSTRING("")); EmptyCString() @@ +90,5 @@ > + nsCString mTerminationReason; > + nsCString mURL; > + const uint16_t mStatus; > + const nsCString mStatusText; > + nsRefPtr<Headers> mHeaders; I think we should ask bz about it but, InternalResponse holds a reference to mHeaders but it's not CCed. Probably it should.

(In reply to Andrea Marchesini (:baku) from comment #9) > Comment on attachment 8496326 [details] [diff] [review] > Patch 1: Request implementation > > Review of attachment 8496326 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/fetch/Fetch.cpp > @@ +27,5 @@ > > + nsCOMPtr<nsIInputStream> byteStream; > > + if (aBodyInit.IsArrayBuffer()) { > > + const ArrayBuffer& buf = aBodyInit.GetAsArrayBuffer(); > > + buf.ComputeLengthAndData(); > > + //XXXnsm reinterpret_cast<> is used in DOMParser, should be ok. > > This comment is for a follow up or is just a normal comment? Just a normal comment.

(In reply to Andrea Marchesini (:baku) from comment #10) > Comment on attachment 8496990 [details] [diff] [review] > Patch 1: interdiff to use Headers::Fill() > > Review of attachment 8496990 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/fetch/Headers.h > @@ -87,5 @@ > > > > virtual JSObject* WrapObject(JSContext* aCx); > > nsISupports* GetParentObject() const { return mOwner; } > > > > - void SwappedFill(Headers* aHeaders); > > There is something missing in this patch. SwappedFill has been removed, but > I don't see any change in Headers.cpp > > ::: dom/fetch/Request.cpp > @@ +201,1 @@ > > if (aRv.Failed()) { > > NS_WARN_IF The only change to Headers.cpp was removing SwappedFill. I'm not sure why it didn't show up though.

Fixed remnant commit marker

Request patch. remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/85663bd538c3

Sketches the fetch algorithm and adds support for blob: and data: URLs on main thread and workers.

Comment on attachment 8499635 [details] [diff] [review] Patch 3: Split Headers into InternalHeaders Review of attachment 8499635 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/fetch/InternalHeaders.cpp @@ +17,5 @@ > +namespace dom { > + > +void > +InternalHeaders::Append(const nsACString& aName, const nsACString& aValue, > + ErrorResult& aRv) indentation @@ +70,5 @@ > +} > + > +void > +InternalHeaders::GetAll(const nsACString& aName, nsTArray<nsCString>& aResults, > + ErrorResult& aRv) const indentation @@ +215,5 @@ > +} > + > +bool > +InternalHeaders::IsForbiddenRequestNoCorsHeader(const nsACString& aName, > + const nsACString* aValue) const indentation ::: dom/fetch/InternalHeaders.h @@ +86,5 @@ > + static bool IsInvalidValue(const nsACString& aValue, ErrorResult& aRv); > + bool IsImmutable(ErrorResult& aRv) const; > + bool IsForbiddenRequestHeader(const nsACString& aName) const; > + bool IsForbiddenRequestNoCorsHeader(const nsACString& aName, > + const nsACString* aValue = nullptr) const; Nit: This seems a bit strange. Can you split it in 2 methods? bool IsForbiddenRequestNoConrsHeader(const nsACString& aName) const; bool IsForbiddenRequestNoConrsHeader(const nsACString& aName, nsACString& aValue) const;

Comment on attachment 8502866 [details] [diff] [review] Patch 4: FetchDriver with blob and data fetching Review of attachment 8502866 [details] [diff] [review]: ----------------------------------------------------------------- I would like to see this patch again. Thanks. ::: content/base/public/nsHostObjectProtocolHandler.h @@ +16,5 @@ > #define MEDIASOURCEURI_SCHEME "mediasource" > #define FONTTABLEURI_SCHEME "moz-fonttable" > #define RTSPURI_SCHEME "rtsp" > > +class PIFileImpl; FileImpl @@ +119,5 @@ > return NS_SUCCEEDED(aUri->SchemeIs(FONTTABLEURI_SCHEME, &isFont)) && isFont; > } > > extern nsresult > +NS_GetBlobForBlobURI(nsIURI* aURI, PIFileImpl** aBlob); Don't expose PIFileImpl but FileImpl. ::: content/base/src/nsHostObjectProtocolHandler.cpp @@ +598,2 @@ > > nsCOMPtr<PIFileImpl> blobImpl = do_QueryInterface(GetDataObject(aURI)); static_cast of this PIFileImpl to FileImpl and return it. ::: dom/fetch/Fetch.cpp @@ +53,5 @@ > + MOZ_ASSERT(aWorkerPrivate); > + aWorkerPrivate->AssertIsOnWorkerThread(); > + } > + > + nsresult NS_IMETHOD_IMP @@ +59,5 @@ > + { > + AssertIsOnMainThread(); > + nsRefPtr<FetchDriver> fetch = new FetchDriver(mRequest); > + nsresult rv = fetch->Fetch(mResolver); > + if (NS_FAILED(rv)) { NS_WARN_IF @@ +60,5 @@ > + AssertIsOnMainThread(); > + nsRefPtr<FetchDriver> fetch = new FetchDriver(mRequest); > + nsresult rv = fetch->Fetch(mResolver); > + if (NS_FAILED(rv)) { > + // FIXME(nsm): Reject promise. follow up? @@ +73,5 @@ > +DOMFetch(nsIGlobalObject* aGlobal, const RequestOrScalarValueString& aInput, > + const RequestInit& aInit, ErrorResult& aRv) > +{ > + nsRefPtr<Promise> p = Promise::Create(aGlobal, aRv); > + if (aRv.Failed()) { NS_WARN_IF @@ +85,5 @@ > + JS::Rooted<JSObject*> jsGlobal(cx, aGlobal->GetGlobalJSObject()); > + GlobalObject global(cx, jsGlobal); > + > + nsRefPtr<Request> request = Request::Constructor(global, aInput, aInit, aRv); > + if (aRv.Failed()) { NS_WARN_IF @@ +89,5 @@ > + if (aRv.Failed()) { > + return nullptr; > + } > + > + nsRefPtr<mozilla::dom::InternalRequest> r = request->GetInternalRequest(); mozilla::dom:: is not needed @@ +98,5 @@ > + if (NS_IsMainThread()) { > + nsRefPtr<MainThreadFetchResolver> resolver = new MainThreadFetchResolver(p); > + nsRefPtr<FetchDriver> fetch = new FetchDriver(r); > + aRv = fetch->Fetch(resolver); > + if (aRv.Failed()) { NS_WARN_IF @@ +105,5 @@ > + } else { > + WorkerPrivate* worker = GetCurrentThreadWorkerPrivate(); > + MOZ_ASSERT(worker); > + nsRefPtr<MainThreadFetchRunnable> run = new MainThreadFetchRunnable(worker, p, r); > + NS_DispatchToMainThread(run); A warning in case this fails. @@ +106,5 @@ > + WorkerPrivate* worker = GetCurrentThreadWorkerPrivate(); > + MOZ_ASSERT(worker); > + nsRefPtr<MainThreadFetchRunnable> run = new MainThreadFetchRunnable(worker, p, r); > + NS_DispatchToMainThread(run); > + return p.forget(); remove this line. @@ +171,5 @@ > +} > + > +WorkerFetchResolver::~WorkerFetchResolver() > +{ > + fprintf(stderr, "NSM WorkerFetchResolver going away\n"); What's this fprintf? @@ +198,5 @@ > +GetRequestReferrer(nsIGlobalObject* aGlobal, const InternalRequest* aRequest) > +{ > + nsCString referrerSource; > + if (aRequest->ReferrerIsURL()) { > + referrerSource = aRequest->ReferrerAsURL(); can you do a return here? It would simplify this function @@ +211,5 @@ > + if (NS_WARN_IF(NS_FAILED(rv))) { > + return EmptyCString(); > + } > + > + nsString referrer; nsAutoString ::: dom/fetch/Fetch.h @@ +7,5 @@ > #define mozilla_dom_Fetch_h > > +#include "nsISupportsImpl.h" > + > +#include "FetchDriver.h" Should it be mozilla/dom/FetchDriver.h ? @@ +29,5 @@ > +class WorkerPrivate; > +} // namespace workers > + > +class WorkerFetchResolver MOZ_FINAL : public FetchDriverObserver > +{ Why are these 2 classes in the header file? Can you move them in the C++ file? @@ +70,5 @@ > + ~MainThreadFetchResolver(); > +}; > + > +already_AddRefed<Promise> > +DOMFetch(nsIGlobalObject* aGlobal, const RequestOrScalarValueString& aInput, CreateFetchPromise? Or a better name. ::: dom/fetch/FetchDriver.cpp @@ +4,5 @@ > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > + > +#include "mozilla/dom/FetchDriver.h" > + > +#include "nsIDOMFile.h" remove this. @@ +33,5 @@ > +FetchDriver::~FetchDriver() > +{ > +} > + > +NS_IMETHODIMP nsresult everywhere. @@ +55,5 @@ > + NS_NewRunnableMethodWithArg<bool>(this, &FetchDriver::ContinueFetch, aCORSFlag); > + return NS_DispatchToCurrentThread(r); > + } > + > + MOZ_CRASH("Synchronous fetch not supported"); What about if mFetchRecursionCount is > 1 ? @@ +63,5 @@ > +FetchDriver::ContinueFetch(bool aCORSFlag) > +{ > + workers::AssertIsOnMainThread(); > + > + nsCString url; nsAutoCString @@ +76,5 @@ > + > + // CSP/mixed content checks. > + /*nsCOMPtr<nsIPrincipal> originPrincipal; > + rv = NS_CheckContentLoadPolicy(mRequest->GetContext(), > + requestURI,*/ Can you write a comment about this? Or remove it @@ +79,5 @@ > + rv = NS_CheckContentLoadPolicy(mRequest->GetContext(), > + requestURI,*/ > + > + nsCString scheme; > + rv = requestURI->GetScheme(scheme); nsAutocString @@ +84,5 @@ > + if (NS_WARN_IF(NS_FAILED(rv))) { > + return FailWithNetworkError(); > + } > + > + nsCString originURL; nsAutoCString @@ +93,5 @@ > + return FailWithNetworkError(); > + } > + > + nsIScriptSecurityManager* ssm = nsContentUtils::GetSecurityManager(); > + nsresult isSameOrigin = ssm->CheckSameOriginURI(requestURI, originURI, false); isSameOrigin seems a boolean, not a nsresult. change name. or use 'rv' @@ +128,5 @@ > + > +NS_IMETHODIMP > +FetchDriver::BasicFetch() > +{ > + nsCString url; nsAutoCString @@ +137,5 @@ > + nullptr, > + nullptr); > + NS_ENSURE_SUCCESS(rv, rv); > + > + nsCString scheme; nsAutoCString @@ +141,5 @@ > + nsCString scheme; > + rv = uri->GetScheme(scheme); > + NS_ENSURE_SUCCESS(rv, rv); > + > + if (scheme.LowerCaseEqualsLiteral("about")) { ?!? @@ +143,5 @@ > + NS_ENSURE_SUCCESS(rv, rv); > + > + if (scheme.LowerCaseEqualsLiteral("about")) { > + } else if (scheme.LowerCaseEqualsLiteral("blob")) { > + nsCOMPtr<PIFileImpl> blobImpl; FileImpl @@ +182,5 @@ > + > + response->SetBody(stream); > + BeginResponse(response); > + return SucceedWithResponse(); > + } else if (scheme.LowerCaseEqualsLiteral("data")) { No need } else { after a return. @@ +183,5 @@ > + response->SetBody(stream); > + BeginResponse(response); > + return SucceedWithResponse(); > + } else if (scheme.LowerCaseEqualsLiteral("data")) { > + nsCString method; nsAutoCString @@ +221,5 @@ > + } > + > + NS_WARN_IF(NS_FAILED(rv)); > + return FailWithNetworkError(); > + } else if (scheme.LowerCaseEqualsLiteral("file")) { why this? At least a comment. @@ +226,5 @@ > + } else if (scheme.LowerCaseEqualsLiteral("http") || > + scheme.LowerCaseEqualsLiteral("https")) { > + // FIXME(nsm): HttpFetch. > + return FailWithNetworkError(); > + } else { No else after a return. @@ +230,5 @@ > + } else { > + return FailWithNetworkError(); > + } > + > + return NS_ERROR_FAILURE; This cannot happen. @@ +237,5 @@ > +NS_IMETHODIMP > +FetchDriver::BeginResponse(InternalResponse* aResponse) > +{ > + MOZ_ASSERT(aResponse); > + nsCString reqURL; nsAutoCString ::: dom/fetch/FetchDriver.h @@ +44,5 @@ > + FetchDriver(const FetchDriver&) MOZ_DELETE; > + FetchDriver& operator=(const FetchDriver&) MOZ_DELETE; > + ~FetchDriver(); > + > + NS_IMETHOD Fetch(bool aCORSFlag); these should be nsresult. All of them. ::: dom/fetch/InternalHeaders.cpp @@ +280,5 @@ > +{ > + nsRefPtr<InternalHeaders> basic = new InternalHeaders(*aHeaders); > + ErrorResult result; > + basic->Delete(NS_LITERAL_CSTRING("Set-Cookie"), result); > + MOZ_ASSERT(!result.Failed()); Write a comment saying that this header cannot be invalid mutable ::: dom/fetch/Request.cpp @@ +171,5 @@ > requestHeaders->Clear(); > > if (request->Mode() == RequestMode::No_cors) { > + if (!request->HasSimpleMethod()) { > + nsCString method; nsAutoCString ::: dom/fetch/Response.h @@ +50,5 @@ > > void > GetUrl(DOMString& aUrl) const > { > + nsCString url; nsAutoCString

Response and Split Headers remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/1aa52e01a6fb remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/6e3f63326f13

Changes from comment 20. I did not understand this one: @@ +221,5 @@ > + } > + > + NS_WARN_IF(NS_FAILED(rv)); > + return FailWithNetworkError(); > + } else if (scheme.LowerCaseEqualsLiteral("file")) { > why this? At least a comment. Are you asking about the empty file: case or the return FailWithNetworkError() requiring a comment?

https://hg.mozilla.org/mozilla-central/rev/1aa52e01a6fb https://hg.mozilla.org/mozilla-central/rev/6e3f63326f13

Comment on attachment 8505924 [details] [diff] [review] Patch 4 interdiff Review of attachment 8505924 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/fetch/Fetch.cpp @@ +233,4 @@ > // The actual referrer policy and stripping is dealt with by HttpBaseChannel, > // this always returns the full API referrer URL of the relevant global. > nsCString > GetRequestReferrer(nsIGlobalObject* aGlobal, const InternalRequest* aRequest) I think this should be better to be: nsresult GetRequestRefereer(nsIGlobalObject* aGlobal, const InternalRequest* aRequest, nsACString& aReferrerSource) { aReferrerSource.Truncate(); @@ +234,5 @@ > // this always returns the full API referrer URL of the relevant global. > nsCString > GetRequestReferrer(nsIGlobalObject* aGlobal, const InternalRequest* aRequest) > { > nsCString referrerSource; nsAutoCString. but actually you don't need it if you change the method. @@ +236,5 @@ > GetRequestReferrer(nsIGlobalObject* aGlobal, const InternalRequest* aRequest) > { > nsCString referrerSource; > if (aRequest->ReferrerIsURL()) { > referrerSource = aRequest->ReferrerAsURL(); aReferrerSource = @@ +245,5 @@ > + if (window) { > + nsCOMPtr<nsIDocument> doc = window->GetExtantDoc(); > + if (doc) { > + nsCOMPtr<nsIURI> docURI = doc->GetDocumentURI(); > + nsCString origin; nsAutoCString @@ +248,5 @@ > + nsCOMPtr<nsIURI> docURI = doc->GetDocumentURI(); > + nsCString origin; > + nsresult rv = nsContentUtils::GetASCIIOrigin(docURI, origin); > + if (NS_WARN_IF(NS_FAILED(rv))) { > + return EmptyCString(); return this error. ::: dom/fetch/FetchDriver.cpp @@ +159,5 @@ > + BeginResponse(response); > + return SucceedWithResponse(); > + } > + return FailWithNetworkError(); > + } extra space.

Patch 4 and bustage fix https://hg.mozilla.org/integration/mozilla-inbound/rev/9cea2a5955d7 https://hg.mozilla.org/integration/mozilla-inbound/rev/10bb0e082744

another bustage followup https://hg.mozilla.org/integration/mozilla-inbound/rev/e7b98ed3a353

https://hg.mozilla.org/mozilla-central/rev/9cea2a5955d7 https://hg.mozilla.org/mozilla-central/rev/10bb0e082744 https://hg.mozilla.org/mozilla-central/rev/e7b98ed3a353

This patch has the following big pieces: HTTP support in FetchDriver, which requires the principal of the caller to be passed. Managing worker lifetime when a fetch() call is in progress. Managing worker lifetime when a Response body is being read. Using nsIPipe to link network streams to Request/Response body streams. Using nsIInputStreamPump to convert Request/Response body streams into respective types.

bug 1107516 is a soft blocker for Fetch that ensures fetches initiated from workers will get automatically canceled when workers go away (I think).

I'm doing a try for Cache that includes this patch: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=197271e28aa7 Note there are build errors in Fetch.cpp on clang: /builds/slave/try-osx64_g-000000000000000000/build/dom/fetch/Fetch.cpp:179:14: error: unused variable 'rv' [-Werror,-Wunused-variable]

My current attempt to fix these compile issues can be found here: https://github.com/wanderview/gecko-patches/blob/dev-sw-cache/fetch_fixes.patch

Also note these are pretty consistent oranges on all platforms: 146 INFO TEST-UNEXPECTED-FAIL | /tests/dom/workers/test/fetch/test_request.html | Worker had an error: SyntaxError: JSON.parse: unexpected character at line 1 column 1 of the JSON data at 0 - expected PASS 147 INFO TEST-UNEXPECTED-ERROR | /tests/dom/workers/test/fetch/test_request.html | called finish() multiple times 175 INFO TEST-UNEXPECTED-FAIL | /tests/dom/workers/test/fetch/test_response.html | Worker had an error: SyntaxError: JSON.parse: unexpected character at line 1 column 1 of the JSON data at 0 - expected PASS 176 INFO TEST-UNEXPECTED-ERROR | /tests/dom/workers/test/fetch/test_response.html | called finish() multiple times

(In reply to Ben Kelly [:bkelly] from comment #32) > Also note these are pretty consistent oranges on all platforms: > > > 146 INFO TEST-UNEXPECTED-FAIL | > /tests/dom/workers/test/fetch/test_request.html | Worker had an error: > SyntaxError: JSON.parse: unexpected character at line 1 column 1 of the JSON > data at 0 - expected PASS > > 147 INFO TEST-UNEXPECTED-ERROR | > /tests/dom/workers/test/fetch/test_request.html | called finish() multiple > times > > 175 INFO TEST-UNEXPECTED-FAIL | > /tests/dom/workers/test/fetch/test_response.html | Worker had an error: > SyntaxError: JSON.parse: unexpected character at line 1 column 1 of the JSON > data at 0 - expected PASS > > 176 INFO TEST-UNEXPECTED-ERROR | > /tests/dom/workers/test/fetch/test_response.html | called finish() multiple > times The JSON failures are due to Bug 1072144. Best to disable the tests for now.

Comment on attachment 8534314 [details] [diff] [review] Patch 5: FetchDriver basic HTTP fetch support Review of attachment 8534314 [details] [diff] [review]: ----------------------------------------------------------------- Seems good to me but I would like to see a feedback from bkelly, if he wants. ::: dom/fetch/Fetch.cpp @@ +159,5 @@ > MOZ_ASSERT(aWorkerPrivate); > aWorkerPrivate->AssertIsOnWorkerThread(); > + if (!aWorkerPrivate->AddFeature(aWorkerPrivate->GetJSContext(), mResolver)) { > + NS_WARNING("Could not add WorkerFetchResolver feature to worker"); > + mResolver->CleanUpUnchecked(); Calling this here will not run it protected by mutex, and this method touches mCleanedUp. @@ +303,5 @@ > + return true; > + } > +}; > + > +class WorkerFetchResponseEndRunnable : public WorkerRunnable MOZ_FINAL @@ +339,5 @@ > > nsRefPtr<WorkerFetchResponseRunnable> r = > + new WorkerFetchResponseRunnable(this, aResponse); > + > + AutoSafeJSContext cx; Can you use AutoJSAPI? @@ +340,5 @@ > nsRefPtr<WorkerFetchResponseRunnable> r = > + new WorkerFetchResponseRunnable(this, aResponse); > + > + AutoSafeJSContext cx; > + r->Dispatch(cx); Dispatch can fail. ::: dom/tests/mochitest/fetch/worker_test_fetch_basic_http.js @@ +17,5 @@ > +var passFiles = [['file_XHR_pass1.xml', 'GET', 200, 'OK', 'text/xml'], > + ['file_XHR_pass2.txt', 'GET', 200, 'OK', 'text/plain'], > + ['file_XHR_pass3.txt', 'GET', 200, 'OK', 'text/plain'], > + ]; > + extra space

Comment on attachment 8534314 [details] [diff] [review] Patch 5: FetchDriver basic HTTP fetch support Review of attachment 8534314 [details] [diff] [review]: ----------------------------------------------------------------- Overall I it looks good, but I do think we want to set a load group now. f- for the null load group issue. ::: dom/fetch/FetchDriver.cpp @@ +298,5 @@ > +NS_IMETHODIMP > +FetchDriver::HttpNetworkFetch() > +{ > + nsRefPtr<InternalRequest> httpRequest = new InternalRequest(*mRequest); > + // FIXME(nsm): Figure out how to tee request's body. I think you can either lose this comment or reference bug 1073231. @@ +300,5 @@ > +{ > + nsRefPtr<InternalRequest> httpRequest = new InternalRequest(*mRequest); > + // FIXME(nsm): Figure out how to tee request's body. > + > + // FIXME(nsm): Http network fetch steps 2-7. It would be nice to have a TODO comment with a bug number for things like this. @@ +321,5 @@ > + uri, > + mPrincipal, > + nsILoadInfo::SEC_NORMAL, > + mRequest->GetContext(), > + nullptr, /* FIXME(nsm): loadgroup */ I think you should allow a load group to be passed in to FetchDriver() constructor. The main thread can then use the doc load group and the worker can use the WorkerPrivate::GetLoadGroup(). If no load group is available (for example is fetch is used by Cache::Add on some random thread), then you can do: nsCOMPtr<nsILoadGroup> loadGroup; rv = NS_NewLoadGroup(getter_AddRefs(loadGroup), mPrincipal); I think we should just do this now instead of a follow-up. It shouldn't be that much code. Also, I think we might need the original load group in order to get Josh's interceptor stuff. @@ +347,5 @@ > + MOZ_ASSERT(!mResponse->IsError()); > + > + /*switch (mResponse->GetStatus()) { > + default: > + }*/ TODO with bug number? @@ +436,5 @@ > +FetchDriver::OnStartRequest(nsIRequest* aRequest, > + nsISupports* aContext) > +{ > + MOZ_ASSERT(!mPipeOutputStream); > + nsresult requestStatus; Any reason to have an extra nsresult variable instead of using rv here? @@ +449,5 @@ > + > + uint32_t status; > + channel->GetResponseStatus(&status); > + > + nsCString statusText; nsAutoCString @@ +468,5 @@ > + // pipe has infinite space, which seems odd, but this is effectively what > + // other APIs like XHR do anyway, since the entire response has to be read. > + // The difference is that in XHR it is immediately converted to text or > + // ArrayBuffer or similar, while in Fetch, we pass around streams until JS > + // requests another type. Instead of referencing what XHR does, can you say something like: "The nsIChannel will continue to buffer data in xpcom events even if we block on a fixed size pipe. It might be possible to suspend the channel and then resume when there is space available, but for now use an infinite pipe to avoid blocking." @@ +482,5 @@ > + // Cancel request. > + return rv; > + } > + > + nsCOMPtr<nsIInputStream> cast = do_QueryInterface(pipeInputStream); If you want nsIInputStream here and a blocking output stream, then just use NS_NewPipe() instead of NS_NewPipe2(). @@ +486,5 @@ > + nsCOMPtr<nsIInputStream> cast = do_QueryInterface(pipeInputStream); > + mResponse->SetBody(cast); > + > + // Try to retarget off main thread. > + nsCOMPtr<nsIEventTarget> sts = do_GetService(NS_STREAMTRANSPORTSERVICE_CONTRACTID); If STS is not available, doesn't that mean we are shutting down? Maybe return an error here instead? ::: dom/fetch/FetchDriver.h @@ +26,5 @@ > NS_INLINE_DECL_THREADSAFE_REFCOUNTING(FetchDriverObserver); > virtual void OnResponseAvailable(InternalResponse* aResponse) = 0; > + // This is triggered after the response's body has been set to a valid > + // stream. ConsumeBody may proceed to read the stream. > + virtual void OnResponseEnd() = 0; Is this comment still correct? I thought we set the stream immediately using a pipe now.

Forgot to add test runner, just needs rs.

Comment on attachment 8546003 [details] [diff] [review] Patch 7: Create channel with a loadgroup Review of attachment 8546003 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, although not sure we want to forget the load group so soon. Are we guaranteed HttpNetworkFetch() only ever happens once per FetchDriver? r+ with that addressed. ::: dom/fetch/FetchDriver.cpp @@ +289,4 @@ > nullptr, /* aCallbacks */ > nsIRequest::LOAD_NORMAL, > ios); > + mLoadGroup = nullptr; Do we really want to forget the load group here? What if a redirect happens?

Comment on attachment 8545541 [details] [diff] [review] Patch 6 - Basic test runner Review of attachment 8545541 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/tests/mochitest/fetch/test_fetch_basic_worker.html @@ +15,5 @@ > +<pre id="test"></pre> > +<script class="testbody" type="text/javascript"> > + > + function runTest() { > + var worker = new Worker("worker_test_fetch_basic.js"); I think this file is missing from the patch?

(In reply to Ben Kelly [:bkelly] from comment #38) > Comment on attachment 8546003 [details] [diff] [review] > Patch 7: Create channel with a loadgroup > > Review of attachment 8546003 [details] [diff] [review]: > ----------------------------------------------------------------- > > Looks good, although not sure we want to forget the load group so soon. Are > we guaranteed HttpNetworkFetch() only ever happens once per FetchDriver? r+ > with that addressed. > > ::: dom/fetch/FetchDriver.cpp > @@ +289,4 @@ > > nullptr, /* aCallbacks */ > > nsIRequest::LOAD_NORMAL, > > ios); > > + mLoadGroup = nullptr; > > Do we really want to forget the load group here? What if a redirect happens? Once we create the channel, we never touch it again. The redirect infrastructure is handled in the CORS bug 1119021, where we don't need the loadgroup.

(In reply to Ben Kelly [:bkelly] from comment #39) > Comment on attachment 8545541 [details] [diff] [review] > Patch 6 - Basic test runner > > Review of attachment 8545541 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/tests/mochitest/fetch/test_fetch_basic_worker.html > @@ +15,5 @@ > > +<pre id="test"></pre> > > +<script class="testbody" type="text/javascript"> > > + > > + function runTest() { > > + var worker = new Worker("worker_test_fetch_basic.js"); > > I think this file is missing from the patch? That file is in Patch 5, I'd forgotten to add this file in patch 5.

Can we add an assert that the load group is not null before the channel is created, then?

Comment on attachment 8546003 [details] [diff] [review] Patch 7: Create channel with a loadgroup Review of attachment 8546003 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/fetch/FetchDriver.cpp @@ +284,5 @@ > uri, > mPrincipal, > nsILoadInfo::SEC_NORMAL, > mRequest->GetContext(), > + mLoadGroup, Just to be clear. Please put a MOZ_ASSERT(mLoadGroup) before this NS_NewChannel() call.

Can we at least submit the tests here to wpt?

I believe the blink team has already written a lot of wpt tests. We're basically waiting for them to be uplifted and then merged to gecko.

(In reply to Andrea Marchesini (:baku) from comment #34) > Comment on attachment 8534314 [details] [diff] [review] > Patch 5: FetchDriver basic HTTP fetch support > > Review of attachment 8534314 [details] [diff] [review]: > ----------------------------------------------------------------- > > Seems good to me but I would like to see a feedback from bkelly, if he wants. > > ::: dom/fetch/Fetch.cpp > @@ +159,5 @@ > > MOZ_ASSERT(aWorkerPrivate); > > aWorkerPrivate->AssertIsOnWorkerThread(); > > + if (!aWorkerPrivate->AddFeature(aWorkerPrivate->GetJSContext(), mResolver)) { > > + NS_WARNING("Could not add WorkerFetchResolver feature to worker"); > > + mResolver->CleanUpUnchecked(); > > Calling this here will not run it protected by mutex, and this method > touches mCleanedUp. It is in the constructor where it was just initialized, and hasn't been dispatched to the main thread yet, so it is guaranteed to not run into sync issues, so the unprotected call seems fine. > > @@ +339,5 @@ > > > > nsRefPtr<WorkerFetchResponseRunnable> r = > > + new WorkerFetchResponseRunnable(this, aResponse); > > + > > + AutoSafeJSContext cx; > > Can you use AutoJSAPI? I don't have a global to init the AutoJSAPI here.

I forgot to upload this patch but it should land before 6 & 7.

Multiple declarations of |nsresult rv| at line 489 is fixed locally.

Comment on attachment 8546934 [details] [diff] [review] Patch 5.1: Set request upload stream and headers. Review of attachment 8546934 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me. Just a few nits. Flag baku for DOM peer sign off. ::: dom/fetch/FetchDriver.cpp @@ +324,5 @@ > mRequest->GetMethod(method); > rv = httpChan->SetRequestMethod(method); > NS_ENSURE_SUCCESS(rv, rv); > + > + nsTArray<InternalHeaders::Entry> headers; Please use nsAutoTArray when allocated on the stack. (I just learned of this...) @@ +326,5 @@ > NS_ENSURE_SUCCESS(rv, rv); > + > + nsTArray<InternalHeaders::Entry> headers; > + mRequest->Headers()->GetEntries(headers); > + for (size_t i = 0; i < headers.Length(); ++i) { nit: uint32_t? @@ +359,5 @@ > } > > + nsCOMPtr<nsIUploadChannel2> uploadChan = do_QueryInterface(chan); > + if (uploadChan) { > + nsCString contentType; nsAutoCString @@ +362,5 @@ > + if (uploadChan) { > + nsCString contentType; > + ErrorResult result; > + mRequest->Headers()->Get(NS_LITERAL_CSTRING("content-type"), contentType, result); > + if (result.Failed()) { Can you add a comment here that this is an error if content-type is missing because we explicitly extract and append the content-type in the Request constructor? @@ +369,5 @@ > + > + nsCOMPtr<nsIInputStream> bodyStream; > + mRequest->GetBody(getter_AddRefs(bodyStream)); > + if (bodyStream) { > + nsCString method; nsAutoCString ::: dom/fetch/InternalHeaders.h @@ +87,5 @@ > static already_AddRefed<InternalHeaders> > CORSHeaders(InternalHeaders* aHeaders); > + > + void > + GetEntries(nsTArray<InternalHeaders::Entry>& aEntries); This can be const.

Comment on attachment 8546003 [details] [diff] [review] Patch 7: Create channel with a loadgroup Flag baku for DOM peer sign off on this one too. Andrea, if you want to look at the test patches, please do. I didn't flag you because they seem quite trivial.

Comment on attachment 8546003 [details] [diff] [review] Patch 7: Create channel with a loadgroup Review of attachment 8546003 [details] [diff] [review]: ----------------------------------------------------------------- r- for the test in sub-sub workers. For the rest is good. ::: dom/fetch/Fetch.cpp @@ +173,5 @@ > if (!mResolver) { > return NS_OK; > } > > nsCOMPtr<nsIPrincipal> principal = mResolver->GetWorkerPrivate()->GetPrincipal(); In theory also this principal can be null. @@ +174,5 @@ > return NS_OK; > } > > nsCOMPtr<nsIPrincipal> principal = mResolver->GetWorkerPrivate()->GetPrincipal(); > + nsCOMPtr<nsILoadGroup> loadGroup = mResolver->GetWorkerPrivate()->GetLoadGroup(); can it be null? In this case, do we want to get the loadGroup of the parent recursively? ::: dom/fetch/FetchDriver.cpp @@ +284,5 @@ > uri, > mPrincipal, > nsILoadInfo::SEC_NORMAL, > mRequest->GetContext(), > + mLoadGroup, for how the patch is written, mLoadGrup and mPrincipal can be null if Fetch API is used in a sub-sub worker. Add a test with this MOZ_ASSERT.

Andrea, I think we are now guaranteed to always have a principal and load group. For example, this assert in WorkerPrivateParent::SetPrincipal() effectively asserts that both are non-null: MOZ_ASSERT(NS_LoadGroupMatchesPrincipal(aLoadGroup, aPrincipal)); Because NS_LoadGroupMatchesPrincipal() returns false if either is null. Note, this changed recently in bug 1107516. Of course, you can still have an nsNullPrincipal, but thats different than nullptr. Is there another case you are worried about not covered by this?

Comment on attachment 8546003 [details] [diff] [review] Patch 7: Create channel with a loadgroup Review of attachment 8546003 [details] [diff] [review]: ----------------------------------------------------------------- Good, previously it was not like this. Thanks bkelly!

Andrea, if you could review 5.1, I can land all the patches in this bug. Thanks!

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=4ee332a6d542 Failures are all due to SW tests that are not enabled on central but were enabled in my local copy.

https://hg.mozilla.org/integration/mozilla-inbound/rev/b4c5a27a1ab4 https://hg.mozilla.org/integration/mozilla-inbound/rev/27b9f23fc9e5 https://hg.mozilla.org/integration/mozilla-inbound/rev/8443140d7fcf https://hg.mozilla.org/integration/mozilla-inbound/rev/c326a68d7bfa https://hg.mozilla.org/integration/mozilla-inbound/rev/dc3c8dd37421

https://hg.mozilla.org/mozilla-central/rev/b4c5a27a1ab4 https://hg.mozilla.org/mozilla-central/rev/27b9f23fc9e5 https://hg.mozilla.org/mozilla-central/rev/8443140d7fcf https://hg.mozilla.org/mozilla-central/rev/c326a68d7bfa https://hg.mozilla.org/mozilla-central/rev/dc3c8dd37421

For reference, now implemented in Chrome Canary (behind a flag): https://twitter.com/jaffathecake/status/555405008829952003

https://treeherder.mozilla.org/#/jobs?repo=try&revision=224f99d4075e I want to see if Bug 1137408 fixes the crashes we've been seeing every time I try to land this.

Fetch specification docs largely complete, and needing review: https://developer.mozilla.org/en-US/docs/Web/API/Fetch_API. Note Interface subpages need review too (and their property/method/Ctor subpages): * https://developer.mozilla.org/en-US/docs/Web/API/GlobalFetch * https://developer.mozilla.org/en-US/docs/Web/API/Headers * https://developer.mozilla.org/en-US/docs/Web/API/Request * https://developer.mozilla.org/en-US/docs/Web/API/Response * https://developer.mozilla.org/en-US/docs/Web/API/Body Supporting doc updates needing review: * https://developer.mozilla.org/en-US/docs/Web/API/FormData (related, updated it) * https://developer.mozilla.org/en-US/docs/Web/API/Worker/Functions_and_classes_available_to_workers#APIs_available_in_workers (added in Fetch and FormData, made the list alphabetical) * https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers and https://developer.mozilla.org/en-US/docs/Web/HTTP/Access_control_CORS (updated to mention new definition in Fetch spec) * https://developer.mozilla.org/en-US/docs/Glossary/Guard (Guard definition written, and some associated glossary entries. Not sure where better to put it, as it's not a specific API feature as such) Areas of concern: * https://developer.mozilla.org/en-US/docs/Web/API/Response/useFinalURL - I'm not really sure of what this is supposed to do or how to use it. A use case suggestion would be great. * https://developer.mozilla.org/en-US/docs/Web/API/Body/formData - I'm not really sure of good use case for this - some help would be great here. * https://developer.mozilla.org/en-US/docs/Web/API/Response/error - doesn't yet seem to be supported anywhere, so I've left a note saying so, and will fill it in later. Is this ok? * https://developer.mozilla.org/en-US/docs/Web/API/Response/redirect - doesn't yet seem to be supported anywhere, so I've left a note saying so, and will fill it in later. Is this ok?

(In reply to Chris Mills (Mozilla, MDN editor) [:cmills] from comment #61) > Fetch specification docs largely complete, and needing review: > https://developer.mozilla.org/en-US/docs/Web/API/Fetch_API. r+ > > Note Interface subpages need review too (and their property/method/Ctor > subpages): > > * https://developer.mozilla.org/en-US/docs/Web/API/GlobalFetch fetch() promise will reject on network error with a typeerror. a network error is usually a permissions issue and such. for example, a 404 is not a network error. So a true check for successful normal fetch would include: 1) Check promise resolved 2) Response.ok should be true. fetch's params are identical to Request's > * https://developer.mozilla.org/en-US/docs/Web/API/Headers > * https://developer.mozilla.org/en-US/docs/Web/API/Request For Request and Response clone() it is important to underline that it will throw if the body has already been used. Also make the case that the purpose of clone() is to get around the fact that bodies are one-time only. context is only really relevant in ServiceWorkers, where the SW can make decisions based on if the URL is going to be used as an image, or an embed or a video or iframe etc. referrer when referrer is internally no-referrer will return an empty string. the CORS mode affects some properties of the fetch as outlined in the spec. it would be nice to have some text on that. The credentials property is slightly badly named (internally it is a credentials mode), it is similar to XHR.withCredentials, but tri-valued. So it does not contain the credentials, but indicates if the UA should send credentials (cookies,auth) to the fetched url. omit will never send, same-origin will only send if the url is same origin with the script and include will include even for cross-origin calls. > * https://developer.mozilla.org/en-US/docs/Web/API/Response FormData body ctor currently unsupported (same for Request options.body), ideally will be supported in 39. (Both have bugs on file just waiting on review) Response's ctor's headers can be passed as Headers objects or object literal of bytestring key-values, not plain bytestring. Response's type "error" never really gets exposed to script since such a fetch() will instead reject the promise. Also, the enums are just strings. Response.redirect() will actually lead to a redirect if a SW sends it upstream. It should be stressed that this is a real redirect. > * https://developer.mozilla.org/en-US/docs/Web/API/Body formData() not available yet on Fx and Cr, but ideally will be in Fx by 39. json() does not return a JSON object, just an object literal. > > Supporting doc updates needing review: > > * https://developer.mozilla.org/en-US/docs/Web/API/FormData (related, > updated it) > * > https://developer.mozilla.org/en-US/docs/Web/API/Worker/ > Functions_and_classes_available_to_workers#APIs_available_in_workers (added > in Fetch and FormData, made the list alphabetical) > * https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers and > https://developer.mozilla.org/en-US/docs/Web/HTTP/Access_control_CORS > (updated to mention new definition in Fetch spec) > * https://developer.mozilla.org/en-US/docs/Glossary/Guard (Guard definition > written, and some associated glossary entries. Not sure where better to put > it, as it's not a specific API feature as such) > > Areas of concern: > > * https://developer.mozilla.org/en-US/docs/Web/API/Response/useFinalURL - > I'm not really sure of what this is supposed to do or how to use it. A use > case suggestion would be great. That is a tricky attribute. Applies only to ServiceWorkers. There is a small example at https://bugzilla.mozilla.org/show_bug.cgi?id=1134352. > * https://developer.mozilla.org/en-US/docs/Web/API/Body/formData - I'm not > really sure of good use case for this - some help would be great here. Very useful for SWs. If a user submits a form, and the SW intercepts it, it could call formData() on the request to obtain a key-value map, modify some fields, the send the form onwards to the server (or use it locally). > * https://developer.mozilla.org/en-US/docs/Web/API/Response/error - doesn't > yet seem to be supported anywhere, so I've left a note saying so, and will > fill it in later. Is this ok? SWs should use this to trigger an error when a controlled page fetches something. > * https://developer.mozilla.org/en-US/docs/Web/API/Response/redirect - > doesn't yet seem to be supported anywhere, so I've left a note saying so, > and will fill it in later. Is this ok? Just like error, this function is useful only for serviceworkers. Thanks for all the docs!

(In reply to Nikhil Marathe [:nsm] (needinfo? please) from comment #62) > (In reply to Chris Mills (Mozilla, MDN editor) [:cmills] from comment #61) > > Fetch specification docs largely complete, and needing review: > > https://developer.mozilla.org/en-US/docs/Web/API/Fetch_API. > > r+ > > > > > Note Interface subpages need review too (and their property/method/Ctor > > subpages): > > > > * https://developer.mozilla.org/en-US/docs/Web/API/GlobalFetch > > fetch() promise will reject on network error with a typeerror. a network > error is usually a permissions issue and such. for example, a 404 is not a > network error. So a true check for successful normal fetch would include: > 1) Check promise resolved > 2) Response.ok should be true. > > fetch's params are identical to Request's This is really useful info to mention: thanks! I've written it up and included it at: https://developer.mozilla.org/en-US/docs/Web/API/GlobalFetch https://developer.mozilla.org/en-US/docs/Web/API/GlobalFetch/fetch > > > * https://developer.mozilla.org/en-US/docs/Web/API/Headers > > * https://developer.mozilla.org/en-US/docs/Web/API/Request > > For Request and Response clone() it is important to underline that it will > throw if the body has already been used. Also make the case that the purpose > of clone() is to get around the fact that bodies are one-time only. Ok. Notes added to https://developer.mozilla.org/en-US/docs/Web/API/Request/clone https://developer.mozilla.org/en-US/docs/Web/API/Response/clone > context is only really relevant in ServiceWorkers, where the SW can make > decisions based on if the URL is going to be used as an image, or an embed > or a video or iframe etc. Note added to https://developer.mozilla.org/en-US/docs/Web/API/Request/context > > referrer when referrer is internally no-referrer will return an empty string. I've added a note about it here: https://developer.mozilla.org/en-US/docs/Web/API/Request/referrer. Although admittedly I don't really understand this ;-) > > the CORS mode affects some properties of the fetch as outlined in the spec. > it would be nice to have some text on that. Can you point me to this info? I'm am not sure what you're referring to. I added cors-with-forced-preflight as an option, as I didn't have that before. > > The credentials property is slightly badly named (internally it is a > credentials mode), it is similar to XHR.withCredentials, but tri-valued. So > it does not contain the credentials, but indicates if the UA should send > credentials (cookies,auth) to the fetched url. omit will never send, > same-origin will only send if the url is same origin with the script and > include will include even for cross-origin calls. Great info, thanks! I've added this to https://developer.mozilla.org/en-US/docs/Web/API/Request/credentials > > > * https://developer.mozilla.org/en-US/docs/Web/API/Response > FormData body ctor currently unsupported updated. (same for Request options.body), > ideally will be supported in 39. (Both have bugs on file just waiting on > review) To clarify, do you mean the body option in the init object of the Request() constructor? > > Response's ctor's headers can be passed as Headers objects or object literal > of bytestring key-values, not plain bytestring. Updated description at https://developer.mozilla.org/en-US/docs/Web/API/Response/Response > > Response's type "error" never really gets exposed to script since such a > fetch() will instead reject the promise. Also, the enums are just strings. Thanks! I've added a description here: https://developer.mozilla.org/en-US/docs/Web/API/Response/type I also added a similar description here: https://developer.mozilla.org/en-US/docs/Web/API/Response/error. Is that relevant there too? > Response.redirect() will actually lead to a redirect if a SW sends it > upstream. It should be stressed that this is a real redirect. Added: https://developer.mozilla.org/en-US/docs/Web/API/Response/redirect > > > > * https://developer.mozilla.org/en-US/docs/Web/API/Body > formData() not available yet on Fx and Cr, but ideally will be in Fx by 39. > json() does not return a JSON object, just an object literal. Description updated - thanks! https://developer.mozilla.org/en-US/docs/Web/API/Body/json > > > > Supporting doc updates needing review: > > > > * https://developer.mozilla.org/en-US/docs/Web/API/FormData (related, > > updated it) > > * > > https://developer.mozilla.org/en-US/docs/Web/API/Worker/ > > Functions_and_classes_available_to_workers#APIs_available_in_workers (added > > in Fetch and FormData, made the list alphabetical) > > * https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers and > > https://developer.mozilla.org/en-US/docs/Web/HTTP/Access_control_CORS > > (updated to mention new definition in Fetch spec) > > * https://developer.mozilla.org/en-US/docs/Glossary/Guard (Guard definition > > written, and some associated glossary entries. Not sure where better to put > > it, as it's not a specific API feature as such) > > > > Areas of concern: > > > > * https://developer.mozilla.org/en-US/docs/Web/API/Response/useFinalURL - > > I'm not really sure of what this is supposed to do or how to use it. A use > > case suggestion would be great. > > That is a tricky attribute. Applies only to ServiceWorkers. There is a small > example at https://bugzilla.mozilla.org/show_bug.cgi?id=1134352. Cool - I've just added this example in, so it seems to make sense. > > > * https://developer.mozilla.org/en-US/docs/Web/API/Body/formData - I'm not > > really sure of good use case for this - some help would be great here. > > Very useful for SWs. If a user submits a form, and the SW intercepts it, it > could call formData() on the request to obtain a key-value map, modify some > fields, the send the form onwards to the server (or use it locally). Description added to the page - thanks. > > > * https://developer.mozilla.org/en-US/docs/Web/API/Response/error - doesn't > > yet seem to be supported anywhere, so I've left a note saying so, and will > > fill it in later. Is this ok? > > SWs should use this to trigger an error when a controlled page fetches > something. Ok, added. > > * https://developer.mozilla.org/en-US/docs/Web/API/Response/redirect - > > doesn't yet seem to be supported anywhere, so I've left a note saying so, > > and will fill it in later. Is this ok? > > Just like error, this function is useful only for serviceworkers. Ok, cool. Again, I've filled in a description. I'll go through and make sure all of the parts that are relevant only to SWs are duely noted.

I'm not sure this bug report is the right place to discuss that but currently, the Fetch API is not available in Javascript modules (which have access to XHR through XPCOM interfaces or to lower level HTTP interfaces). Is it on purpose ?

(In reply to Axel from comment #64) > I'm not sure this bug report is the right place to discuss that but > currently, the Fetch API is not available in Javascript modules (which have > access to XHR through XPCOM interfaces or to lower level HTTP interfaces). > Is it on purpose ? As far as I know, adding [Exposed=System] to the relevant webidl interfaces will fix this. Specifically, add [Exposed=System] to Request, Response, Headers. I'm not sure how to expose the fetch() function. Probably something similar to how Promise is exposed.