Closed Bug 1065216 Opened 10 years ago Closed 9 years ago

Implement fetch event for ServiceWorkers

Categories

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

Product:
Core
Component:
DOM: Core & HTML
Platform:
x86
macOS
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla38
Tracking Flags:
Tracking Status
firefox38 --- fixed

People

(Reporter: jdm, Assigned: jdm)

References

(
URL
)

Details

Attachments

(1 file, 20 obsolete files)

Dispatch a fetch event to workers when controlled pages initiate a network load
80.34 KB, patch
Details | Diff | Splinter Review 
This includes things like FetchEvent and [[HandleFetch]].
Just attaching a WIP for posterity. Ignoring the event causes loads to continue; calling event.respondWith(new Promise(function(){})) causes the load to suspend indefinitely.
Assignee: nobody → josh
Status: NEW → ASSIGNED
Blocks: 983499
Depends on: 1017613
Depends on: NavigationController
Attachment #8486913 - Attachment is obsolete: true
The IDL for the event init dictionary should have things either with default values or required.

Or (less desirable) the spec needs to define what happens when those properties are missing or something.

But more generally, this API is not making sense to me.  Specifically:

1)  While the event object is alive, it entrains the request in question, because it must
    allow multiple default() calls, right?

2)  forwardTo is not specified at all.

3)  The respondWith definition makes no sense either.  It seems to block the caller until
    the promise settles (but processes other events in the caller?), but then doesn't do
    anything with the response.  Instead, [[HandleFetch]] is supposed to get this value
    somehow through some unspecified backdoor.

4)  [[HandleFetch]] requires dispatch for this event that differs from how every single
    other event in the platform is dispatched.  Specifically, it requires that processing
    steps of some sort be interjected into the event dispatch.  As far as I can tell,
    these processing steps, listed in step 2, are supposed to come during the middle of
    step 1, and can involve "waiting" (which typically means "yield and spin the event
    loop", but doing that in the middle of event dispatch seems like a bad idea).

I think we should be pushing back on this spec to become sane.
Oh, and it's not clear to me what the exception handling there in [[HandleFetch]] is supposed to do.  Does "abort these steps" mean the toplevel [[HandleFetch]] algorithm, the "For each event listener invoked" steps, or something else?
Anne, Jake, comment 3 & 4.
Flags: needinfo?(jaffathecake)
Flags: needinfo?(annevk)
Jungkee, see above.
Flags: needinfo?(annevk)
Now with a vaguely useful Request object and working respondWith method.
Attachment #8486914 - Attachment is obsolete: true
Attachment #8488064 - Attachment is obsolete: true
Comment on attachment 8488117 [details] [diff] [review]
Dispatch a fetch event to workers when controlled pages initiate a network load (WIP)

Want to tell me if this looks sensible right now?
Attachment #8488117 - Flags: feedback?(amarchesini)
Note to self: ServiceWorkerClient exists on maple now.
Rebased on the new patches in bug 898524.
Attachment #8495449 - Flags: review?(amarchesini)
Attachment #8488117 - Attachment is obsolete: true
Attachment #8488117 - Flags: feedback?(amarchesini)
Comment on attachment 8495449 [details] [diff] [review]
Dispatch a fetch event to workers when controlled pages initiate a network load (WIP)

Just looking for feedback at the moment.
Attachment #8495449 - Flags: review?(amarchesini) → feedback?(amarchesini)
I've got a local patch that adds the ServiceWorkerClient field, too.
Depends on: 982726
Comment on attachment 8495449 [details] [diff] [review]
Dispatch a fetch event to workers when controlled pages initiate a network load (WIP)

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

It seems ok so far. No big issues.
I'm not familiar with nsDocShell and nsHttpChannel, and I would like to have somebody else to review that part when the patch is ready.

::: dom/fetch/FetchBodyStream.h
@@ +65,5 @@
>    virtual ~FetchBodyStream();
>  
>    nsISupports* mOwner;
> +  nsString mTextBody;
> +  bool mHasText;

do you need this extra boolean? Maybe can just use mTextBody.IslVoid()/Empty()

::: dom/fetch/Request.cpp
@@ +77,1 @@
>                       const RequestInit& aInit, ErrorResult& rv)

aRv

::: dom/fetch/Request.h
@@ +64,5 @@
>    already_AddRefed<Headers> Headers() const;
>    already_AddRefed<FetchBodyStream> Body() const;
>  
>    static already_AddRefed<Request>
> +  Constructor(nsISupports* global, const RequestOrString& aInput,

aGlobal and aRv

::: dom/workers/ServiceWorkerEvents.cpp
@@ +51,5 @@
> +  mChannel = aChannel;
> +}
> +
> +/*static*/ already_AddRefed<FetchEvent>
> +FetchEvent::Constructor(mozilla::dom::EventTarget* aOwner,

probably you don't need mozilla::dom

@@ +63,5 @@
> +  if (aOptions.mRequest.WasPassed()) {
> +    e->mRequest = new dom::Request(aOptions.mRequest.Value());
> +  }
> +  e->mContext = aOptions.mContext.WasPassed() ?
> +      aOptions.mContext.Value() : mozilla::dom::Context::Connect;

same here

@@ +69,5 @@
> +      aOptions.mIsReload.Value() : false;
> +  return e.forget();
> +}
> +
> +class TextHandler MOZ_FINAL : public PromiseNativeHandler {

{ new line.

@@ +83,5 @@
> +
> +  void
> +  RejectedCallback(JSContext* aCx, JS::Handle<JS::Value> aValue) MOZ_OVERRIDE;
> +};
> +

Can these classes go to an anonymous namespace?

::: dom/workers/ServiceWorkerEvents.h
@@ +33,5 @@
> +  nsCOMPtr<nsISupports> mJunk;
> +  nsMainThreadPtrHandle<nsIInterceptedChannel> mChannel;
> +  OwningNonNull<mozilla::dom::Request> mRequest;
> +  mozilla::dom::Context mContext;
> +  bool mIsReload;

Not initialized in the ctor.

::: dom/workers/ServiceWorkerManager.cpp
@@ +2014,5 @@
> +
> +    nsCOMPtr<nsIChannel> channel;
> +    mInterceptedChannel->GetChannel(getter_AddRefs(channel));
> +    nsCOMPtr<nsIURI> uri;
> +    channel->GetURI(getter_AddRefs(uri));

return value?

@@ +2015,5 @@
> +    nsCOMPtr<nsIChannel> channel;
> +    mInterceptedChannel->GetChannel(getter_AddRefs(channel));
> +    nsCOMPtr<nsIURI> uri;
> +    channel->GetURI(getter_AddRefs(uri));
> +    uri->GetSpec(mSpec);

return value?

@@ +2018,5 @@
> +    channel->GetURI(getter_AddRefs(uri));
> +    uri->GetSpec(mSpec);
> +
> +    nsCOMPtr<nsIHttpChannel> httpChannel = do_QueryInterface(channel);
> +    httpChannel->GetRequestMethod(mMethod);

here as well.

@@ +2065,5 @@
> +    reqInit.mHeaders.Construct();
> +    reqInit.mHeaders.Value().SetAsHeaders() = headers;
> +
> +    reqInit.mBody.Construct();
> +    reqInit.mBody.Value().SetAsString() = NS_LITERAL_STRING(""); //XXXjdm get this from the channel

In the meantime: EmptyString() ?
Attachment #8495449 - Flags: feedback?(amarchesini) → feedback+
Rebased on top of nsm's Request and Response implementation, with baku's comments addressed.
Attachment #8495449 - Attachment is obsolete: true
Comment on attachment 8502663 [details] [diff] [review]
Dispatch a fetch event to workers when controlled pages initiate a network load (WIP)

Olli, could you review the changes to nsDocument/nsDocShell?
Attachment #8502663 - Flags: review?(bugs)
Comment on attachment 8502663 [details] [diff] [review]
Dispatch a fetch event to workers when controlled pages initiate a network load (WIP)

Honza, could you review the netwerk/ changes for intercepted channels?
Attachment #8502663 - Flags: review?(honzab.moz)
Whoops, forgot to complete the documentation in nsINetworkInterceptController.idl.
Attachment #8502663 - Attachment is obsolete: true
Attachment #8502663 - Flags: review?(honzab.moz)
Attachment #8502663 - Flags: review?(bugs)
Comment on attachment 8502795 [details] [diff] [review]
Dispatch a fetch event to workers when controlled pages initiate a network load (WIP)

Same request as previously.
Attachment #8502795 - Flags: review?(bugs)
Comment on attachment 8502795 [details] [diff] [review]
Dispatch a fetch event to workers when controlled pages initiate a network load (WIP)

Same request as previously.
Attachment #8502795 - Flags: review?(honzab.moz)
Comment on attachment 8502795 [details] [diff] [review]
Dispatch a fetch event to workers when controlled pages initiate a network load (WIP)

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

r=honzab for the networking parts.
Attachment #8502795 - Flags: review?(honzab.moz) → review+
Comment on attachment 8502795 [details] [diff] [review]
Dispatch a fetch event to workers when controlled pages initiate a network load (WIP)


2 spaces for indentation in nsDocument
Attachment #8502795 - Flags: review?(bugs) → review+
jdm, any particular bits preventing landing this on m-c?
Yes; the patch currently supports ServiceWorkerClient, which hasn't landed on m-c yet. I also can't find the appropriate bug to mark that dependency.
Here's a patch which is a rollup of my most recent maple changes. It doesn't build against mozilla-central for the aforementioned reasons. I still need to get more netwerk/ review for the channel cancellation changes.
Attachment #8509903 - Flags: review?(amarchesini)
Attachment #8502795 - Attachment is obsolete: true
Comment on attachment 8509903 [details] [diff] [review]
Dispatch a fetch event to workers when controlled pages initiate a network load

Jason, could you look at the netwerk/ changes since Honza's away?
Attachment #8509903 - Flags: review?(jduell.mcbugs)
Comment on attachment 8509903 [details] [diff] [review]
Dispatch a fetch event to workers when controlled pages initiate a network load

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

I leave the necko review part to jduell.

r- just because I want to see the patch again with some MOZ_ASSERT() for the thread.
Plus because I want to check it again. It's not easy code.

::: content/base/src/nsDocument.cpp
@@ +3164,5 @@
> +
> +NS_IMETHODIMP
> +nsDocument::ChannelIntercepted(nsIInterceptedChannel* aChannel)
> +{
> +  nsCOMPtr<nsIServiceWorkerManager> swm = mozilla::services::GetServiceWorkerManager();

if (!swm) {... ?

::: content/base/src/nsDocument.h
@@ +624,3 @@
>  #undef DECL_SHIM
>    };
>    

just because you are here, can you remove this 2 extra spaces?

::: dom/webidl/FetchEvent.webidl
@@ +11,5 @@
> + Func="mozilla::dom::workers::ServiceWorkerEventsVisible",
> + Exposed=(ServiceWorker)]
> +interface FetchEvent : Event {
> +  readonly attribute Request request;
> +  // readonly attribute ServiceWorkerClient client; // The window issuing the request.

do we have a follow up for this? or at least a bug id?

::: dom/workers/ServiceWorkerEvents.cpp
@@ +44,5 @@
> +{
> +}
> +
> +FetchEvent::~FetchEvent()
> +{

Can you add MOZ_ASSERT(the correct thread) ? this object runs in a particular thread, so we should be sure we don't use it, or call some method in the wrong thread.
Would be nice to have this check in any method, if not thread-safe.

@@ +60,5 @@
> +                        const nsAString& aType,
> +                        const FetchEventInit& aOptions,
> +                        ErrorResult& aRv)
> +{
> +  nsRefPtr<EventTarget> owner = do_QueryObject(aGlobal.GetAsSupports());

MOZ_ASSERT(owner);

@@ +66,5 @@
> +  bool trusted = e->Init(owner);
> +  e->InitEvent(aType, aOptions.mBubbles, aOptions.mCancelable);
> +  e->SetTrusted(trusted);
> +  e->mRequest = aOptions.mRequest.WasPassed() ?
> +      &aOptions.mRequest.Value() : nullptr;;

double ';'

@@ +111,5 @@
> +  void
> +  RejectedCallback(JSContext* aCx, JS::Handle<JS::Value> aValue) MOZ_OVERRIDE;
> +};
> +
> +struct RespondWithClosure {

{ new line

@@ +120,5 @@
> +  {
> +  }
> +};
> +
> +void RespondWithCopyComplete(void* closure, nsresult status)

1. aClosure and aStatus

2. you don't use 'status' at all, correct?

@@ +128,5 @@
> +  NS_DispatchToMainThread(event);
> +}
> +
> +class AutoCancel
> +{

MOZ_STACK_CLASS

@@ +133,5 @@
> +  nsRefPtr<RespondWithHandler> mOwner;
> +
> +public:
> +  AutoCancel(RespondWithHandler* aOwner)
> +  : mOwner(aOWner)

aOWner -> aOwner

I don't think this patch compiles... does it?

@@ +140,5 @@
> +
> +  ~AutoCancel()
> +  {
> +    if (mOwner) {
> +      mOwner->CancelRequest();

CancelRequest is private, it cannot be called by this object. Make it public.

@@ +155,5 @@
> +RespondWithHandler::ResolvedCallback(JSContext* aCx, JS::Handle<JS::Value> aValue)
> +{
> +  AutoCancel autoCancel(this);
> +
> +  if (!aValue.isObject()) {

NS_WARN_IF

@@ +161,5 @@
> +  }
> +
> +  nsRefPtr<Response> response;
> +  nsresult rv = UNWRAP_OBJECT(Response, &aValue.toObject(), response);
> +  if (NS_FAILED(rv)) {

NS_WARN_IF

@@ +170,5 @@
> +  response->GetBody(getter_AddRefs(body));
> +
> +  nsCOMPtr<nsIOutputStream> responseBody;
> +  rv = mInterceptedChannel->GetResponseBody(getter_AddRefs(responseBody));
> +  if (NS_FAILED(rv)) {

NS_WARN_IF

@@ +176,5 @@
> +  }
> +
> +  nsAutoPtr<RespondWithClosure> closure(new RespondWithClosure(mInterceptedChannel));
> +
> +  nsCOMPtr<nsIEventTarget> stsThread = do_GetService(NS_SOCKETTRANSPORTSERVICE_CONTRACTID, &rv);

if (!stsThread)

@@ +179,5 @@
> +
> +  nsCOMPtr<nsIEventTarget> stsThread = do_GetService(NS_SOCKETTRANSPORTSERVICE_CONTRACTID, &rv);
> +  rv = NS_AsyncCopy(body, responseBody, stsThread, NS_ASYNCCOPY_VIA_READSEGMENTS, 4096,
> +                    RespondWithCopyComplete, closure.forget());
> +  if (NS_FAILED(rv)) {

NS_WARN_IF

@@ +183,5 @@
> +  if (NS_FAILED(rv)) {
> +    return;
> +  }
> +
> +  autoCancel->Reset();

-> vs . :)

@@ +186,5 @@
> +
> +  autoCancel->Reset();
> +}
> +
> +class CancelChannelRunnable: public nsRunnable {

MOZ_FINAL ?

@@ +196,5 @@
> +  }
> +
> +  NS_IMETHOD Run()
> +  {
> +    MOZ_ASSERT(NS_IsMainThread());

can we call mChannel->Cancel() twice?

@@ +211,5 @@
> +}
> +
> +void
> +RespondWithHandler::CancelRequest()
> +{

do we care to know if we've already canceled this request?
I'm wondering if a check can be useful.

@@ +241,5 @@
> +  nsRefPtr<Promise> promise = Promise::Create(global, result);
> +  if (result.Failed()) {
> +    return nullptr;
> +  }
> +

So, if this is the only thing we want to do, just mark ForwardTo as [throws] in the webidl file and do:

already_Addref<Promise>
FetchEvent::ForwardTo(const nsAString& aUrl, ErrorResult& aRv)
{
  aRv.Throws(NS_ERROR_NOT_AVAILABLE);
  return nullptr;
}

@@ +256,5 @@
> +  nsRefPtr<Promise> promise = Promise::Create(global, result);
> +  if (result.Failed()) {
> +    return nullptr;
> +  }
> +

Same here.

::: dom/workers/ServiceWorkerEvents.h
@@ +31,5 @@
> +class FetchEvent : public Event
> +{
> +  nsMainThreadPtrHandle<nsIInterceptedChannel> mChannel;
> +  nsRefPtr<mozilla::dom::Request> mRequest;
> +  mozilla::dom::Context mContext;

using namespace mozilla:: dom ?

::: dom/workers/ServiceWorkerManager.cpp
@@ +2036,5 @@
> +    NS_ENSURE_SUCCESS(rv, rv);
> +
> +    //XXXjdm not sure this is a good heuristic - we should probably include reload-ness
> +    //       in the loadinfo or as a separate load flag
> +    //mIsReload = loadFlags & (HttpBaseChannel::LOAD_BYPASS_CACHE | HttpBaseChannel::LOAD_FRESH_CONNECTION);

I don't know about this. Can you ask to somebody who should know more about HttpBaseChannel?

@@ +2050,5 @@
> +    return DispatchFetchEvent(aCx, aWorkerPrivate);
> +  }
> +
> +private:
> +  class ResumeRequest : public nsRunnable {

MOZ_FINAL?

@@ +2093,5 @@
> +    reqInit.mCredentials.Construct(RequestCredentials::Same_origin); //XXXjdm how to find this out?
> +
> +    ErrorResult rv;
> +    nsRefPtr<Request> request = Request::Constructor(globalObj, requestInfo, reqInit, rv);
> +    if (rv.Failed()) {

NS_WARN_IF

@@ +2097,5 @@
> +    if (rv.Failed()) {
> +      return false;
> +    }
> +
> +    FetchEventInit init;

I don't know if this is needed, but maybe you can do:
RootedDictionary<FetchEventInit> init(aCx); ?

@@ +2143,5 @@
> +      new FetchEventRunnable(sw->GetWorkerPrivate(), handle, windowId);
> +  rv = event->Init();
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  AutoSafeJSContext cx;

can you use AutoJAPI(cx, the global from the window?)
Attachment #8509903 - Flags: review?(amarchesini)
We should remove Context pending resolution of https://github.com/slightlyoff/ServiceWorker/issues/497. The name is also generic and causes compilation errors on b2g.

We also don't expose RequestContext on Request right now, that will be another bug.
Comment on attachment 8509903 [details] [diff] [review]
Dispatch a fetch event to workers when controlled pages initiate a network load

jdm, sorry I didn't get to this.  Honza is back and understands this code better than I do.
Attachment #8509903 - Flags: review?(jduell.mcbugs) → review?(honzab.moz)
Comment on attachment 8509903 [details] [diff] [review]
Dispatch a fetch event to workers when controlled pages initiate a network load

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

::: netwerk/protocol/http/InterceptedChannel.cpp
@@ +176,5 @@
> +  if (!mChannel) {
> +    return NS_ERROR_FAILURE;
> +  }
> +
> +  nsresult rv = mChannel->AsyncAbort(NS_BINDING_ABORTED);

You can just Cancel() the channel (AsyncAbort btw duplicates OnStart/OnStop and is racy).  Applies to the child part as well.
Attachment #8509903 - Flags: review?(honzab.moz) → review-
Comment on attachment 8509903 [details] [diff] [review]
Dispatch a fetch event to workers when controlled pages initiate a network load

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

::: netwerk/protocol/http/InterceptedChannel.cpp
@@ +176,5 @@
> +  if (!mChannel) {
> +    return NS_ERROR_FAILURE;
> +  }
> +
> +  nsresult rv = mChannel->AsyncAbort(NS_BINDING_ABORTED);

Based on private chat with jdm who refreshed my memory how this all works, calling AsyncAbort is alright.

Just add comments why it is so, since this code is rather complicated and I won't be the only one lost here ;)
Attachment #8509903 - Flags: review- → review+
(In reply to Andrea Marchesini (:baku) from comment #28)
> ::: dom/webidl/FetchEvent.webidl
> @@ +11,5 @@
> > + Func="mozilla::dom::workers::ServiceWorkerEventsVisible",
> > + Exposed=(ServiceWorker)]
> > +interface FetchEvent : Event {
> > +  readonly attribute Request request;
> > +  // readonly attribute ServiceWorkerClient client; // The window issuing the request.
> 
> do we have a follow up for this? or at least a bug id?

Bug 982726 has landed, so I've re-enabled this.
 
> ::: dom/workers/ServiceWorkerEvents.cpp
> @@ +44,5 @@
> > +{
> > +}
> > +
> > +FetchEvent::~FetchEvent()
> > +{
> 
> Can you add MOZ_ASSERT(the correct thread) ? this object runs in a
> particular thread, so we should be sure we don't use it, or call some method
> in the wrong thread.
> Would be nice to have this check in any method, if not thread-safe.

I don't see the purpose here, as this seems to duplicate the work of non-threadsafe nsISupports checks.

> @@ +155,5 @@
> > +RespondWithHandler::ResolvedCallback(JSContext* aCx, JS::Handle<JS::Value> aValue)
> > +{
> > +  AutoCancel autoCancel(this);
> > +
> > +  if (!aValue.isObject()) {
> 
> NS_WARN_IF

This is an expected failure that web content can trigger.

> @@ +161,5 @@
> > +  }
> > +
> > +  nsRefPtr<Response> response;
> > +  nsresult rv = UNWRAP_OBJECT(Response, &aValue.toObject(), response);
> > +  if (NS_FAILED(rv)) {
> 
> NS_WARN_IF

Expected failure that web content can trigger.
 
> @@ +196,5 @@
> > +  }
> > +
> > +  NS_IMETHOD Run()
> > +  {
> > +    MOZ_ASSERT(NS_IsMainThread());
> 
> can we call mChannel->Cancel() twice?

We probably could before, and now we definitely can.
 
> @@ +211,5 @@
> > +}
> > +
> > +void
> > +RespondWithHandler::CancelRequest()
> > +{
> 
> do we care to know if we've already canceled this request?
> I'm wondering if a check can be useful.

No.
 
> @@ +241,5 @@
> > +  nsRefPtr<Promise> promise = Promise::Create(global, result);
> > +  if (result.Failed()) {
> > +    return nullptr;
> > +  }
> > +
> 
> So, if this is the only thing we want to do, just mark ForwardTo as [throws]
> in the webidl file and do:
> 
> already_Addref<Promise>
> FetchEvent::ForwardTo(const nsAString& aUrl, ErrorResult& aRv)
> {
>   aRv.Throws(NS_ERROR_NOT_AVAILABLE);
>   return nullptr;
> }

I'm not convinced that this is the best strategy. It seems like it would be preferable for compat purposes to fulfill the expected API in ways that won't break any content that tries to use it properly.
 
> ::: dom/workers/ServiceWorkerEvents.h
> @@ +31,5 @@
> > +class FetchEvent : public Event
> > +{
> > +  nsMainThreadPtrHandle<nsIInterceptedChannel> mChannel;
> > +  nsRefPtr<mozilla::dom::Request> mRequest;
> > +  mozilla::dom::Context mContext;
> 
> using namespace mozilla:: dom ?

Not in a header.
Attached patch Interdiff (obsolete) — — Splinter Review 
This is the interdiff addressing review comments so far. The exception is that I added request header support to the FetchEvent's Request object, but accidentally folded it into the original patch.
Rebased and with more comments addressed (also request headers).
Attachment #8509903 - Attachment is obsolete: true
Needinfo doesn't appear to be doing much good.
Flags: needinfo?(jaffathecake)
I'm inclined to file followup bugs for providing real values for the body, mode and credential fields of Request, as well as for FetchEvent.isReload, and the Default and ForwardTo methods.
Comment on attachment 8520821 [details] [diff] [review]
Dispatch a fetch event to workers when controlled pages initiate a network load

The interdiff should suffice here, except for the addition of the Request header values that I added to this patch.
Attachment #8520821 - Flags: review?(amarchesini)
The spec is unclear ("Let r be a new Request object associated with request.") whether setting headers on the request object and then allowing it to proceed should add the new headers to the network request, but I propose also punting that to a followup.
Re comment 39, that should not work. The service worker specification should set Headers object's guard to /immutable/.
Note to self: investigate whether https://github.com/slightlyoff/ServiceWorker/issues/560 is problematic for us (cross-origin fetch for same-origin request) and address the issue of multiple waitUntil calls (https://github.com/slightlyoff/ServiceWorker/issues/540).
jdm,

This patch allows distinguishing between fetch and navigate so that the right SW can be used. It has some other crud, sorry about that.
I would like some help integrating this nicely into the InterceptedChannel/HttpChannel design. Primarily, a clean way to convey to both ShouldPrepareForIntercept() and ChannelIntercepted():
a) The URI to intercept. ShouldPrepareForIntercept() gets this, but ChannelIntercepted() has to extract it from the channel.
b) A boolean flag to distinguish fetch/navigate so the right SW can be used. Again, I'd like to avoid ChannelIntercepted() having to do it's own forensics.

The two functions are called via different code paths over different classes, and I'd like to avoid adding a boolean flag everywhere if it is possible to pull out the required parameters in a clean way.
Attachment #8523967 - Flags: feedback?(josh)
Comment on attachment 8520821 [details] [diff] [review]
Dispatch a fetch event to workers when controlled pages initiate a network load

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

It seems good to me and I give you r+, but can you ask a feedback to some necko guy about the use of the channels? I'm not confident enough about that part.
Plus I guess you have to do a couple of follow ups. Right?

::: dom/base/nsDocument.cpp
@@ +3159,5 @@
>  
>  NS_IMETHODIMP
> +nsDocument::ShouldPrepareForIntercept(nsIURI* aURI, bool* aShouldIntercept)
> +{
> +    nsCOMPtr<nsIServiceWorkerManager> swm = mozilla::services::GetServiceWorkerManager();

2 spaces indentation.

@@ +3169,5 @@
> +}
> +
> +NS_IMETHODIMP
> +nsDocument::ChannelIntercepted(nsIInterceptedChannel* aChannel)
> +{

same here.

::: dom/bindings/Bindings.conf
@@ +433,5 @@
>      },
>  },
>  
> +'FetchEvent': {
> +    'headerFile': 'ServiceWorkerEvents.h',

mozilla/dom/ ? or mozilla/dom/workers/

::: dom/workers/ServiceWorkerEvents.cpp
@@ +96,5 @@
> +    return NS_OK;
> +  }
> +};
> +
> +class FinishResponse : public nsRunnable

MOZ_FINAL?

@@ +189,5 @@
> +  }
> +
> +  nsRefPtr<Response> response;
> +  nsresult rv = UNWRAP_OBJECT(Response, &aValue.toObject(), response);
> +  if (NS_FAILED(rv)) {

NS_WARN_IF

@@ +210,5 @@
> +    return;
> +  }
> +  rv = NS_AsyncCopy(body, responseBody, stsThread, NS_ASYNCCOPY_VIA_READSEGMENTS, 4096,
> +                    RespondWithCopyComplete, closure.forget());
> +  if (NS_FAILED(rv)) {

NS_WARN_IF

@@ +262,5 @@
> +  nsCOMPtr<nsIGlobalObject> global = do_QueryInterface(GetParentObject());
> +  MOZ_ASSERT(global);
> +  ErrorResult result;
> +  nsRefPtr<Promise> promise = Promise::Create(global, result);
> +  if (result.Failed()) {

NS_WARN_IF

@@ +277,5 @@
> +  nsCOMPtr<nsIGlobalObject> global = do_QueryInterface(GetParentObject());
> +  MOZ_ASSERT(global);
> +  ErrorResult result;
> +  nsRefPtr<Promise> promise = Promise::Create(global, result);
> +  if (result.Failed()) {

NS_WARN_IF

::: dom/workers/ServiceWorkerEvents.h
@@ +31,5 @@
>  
> +class FetchEvent : public Event
> +{
> +  nsMainThreadPtrHandle<nsIInterceptedChannel> mChannel;
> +  nsRefPtr<mozilla::dom::workers::ServiceWorkerClient> mClient;

mozilla::dom::workers is not needed.

@@ +32,5 @@
> +class FetchEvent : public Event
> +{
> +  nsMainThreadPtrHandle<nsIInterceptedChannel> mChannel;
> +  nsRefPtr<mozilla::dom::workers::ServiceWorkerClient> mClient;
> +  nsRefPtr<mozilla::dom::Request> mRequest;

mozilla::dom:: is not needed.

@@ +37,5 @@
> +  uint64_t mWindowId;
> +  bool mIsReload;
> +  bool mWaitToRespond;
> +protected:
> +  explicit FetchEvent(mozilla::dom::EventTarget* aOwner);

mozilla::dom:: is not needed.

@@ +47,5 @@
> +  NS_FORWARD_TO_EVENT
> +
> +  virtual JSObject* WrapObjectInternal(JSContext* aCx) MOZ_OVERRIDE
> +  {
> +    return mozilla::dom::FetchEventBinding::Wrap(aCx, this);

same here.

@@ +68,5 @@
> +
> +  already_AddRefed<mozilla::dom::Request>
> +  Request()
> +  {
> +    nsRefPtr<mozilla::dom::Request> req = mRequest.get();

same here.

@@ +72,5 @@
> +    nsRefPtr<mozilla::dom::Request> req = mRequest.get();
> +    return req.forget();
> +  }
> +
> +  already_AddRefed<mozilla::dom::workers::ServiceWorkerClient>

same here.

::: dom/workers/ServiceWorkerManager.cpp
@@ +2028,5 @@
> +  mValues.AppendElement(aValue);
> +  return NS_OK;
> +}
> +
> +class FetchEventRunnable : public WorkerRunnable {

1. MOZ_FINAL?
2. { in a new line.

@@ +2041,5 @@
> +  explicit FetchEventRunnable(WorkerPrivate* aWorkerPrivate,
> +                              nsMainThreadPtrHandle<nsIInterceptedChannel>& aChannel,
> +                              uint64_t aWindowId)
> +  : WorkerRunnable(aWorkerPrivate, WorkerThreadModifyBusyCount)
> +  , mInterceptedChannel(aChannel)

mIsReload is not initialized here.

@@ +2114,5 @@
> +    MOZ_ASSERT(aWorkerPrivate->IsServiceWorker());
> +    GlobalObject globalObj(aCx, aWorkerPrivate->GlobalScope()->GetWrapper());
> +
> +    RequestOrScalarValueString requestInfo;
> +    *requestInfo.SetAsScalarValueString().ToAStringPtr() = NS_ConvertUTF8toUTF16(mSpec);

I had a similar issue in another patch and in the end I preferred to use SetAsRequest() = new Request(...); just to avoid to touch FakeStrings. Can you ask a feedback to bz about this?

@@ +2124,5 @@
> +    MOZ_ASSERT(mHeaderNames.Length() == mHeaderValues.Length());
> +    for (uint32_t i = 0; i < mHeaderNames.Length(); i++) {
> +      ErrorResult rv;
> +      internalHeaders->Set(mHeaderNames[i], mHeaderValues[i], rv);
> +      if (rv.Failed()) {

NS_WARN_IF

@@ +2137,5 @@
> +    reqInit.mBody.Construct();
> +    reqInit.mBody.Value().SetAsScalarValueString() = EmptyString(); //XXXjdm get this from the channel
> +
> +    reqInit.mMode.Construct(RequestMode::Same_origin); //XXXjdm how to find this out?
> +    reqInit.mCredentials.Construct(RequestCredentials::Same_origin); //XXXjdm how to find this out?

All of these are followups, right?

@@ +2186,5 @@
> +  rv = aChannel->GetChannel(getter_AddRefs(channel));
> +  if (NS_WARN_IF(NS_FAILED(rv))) {
> +    return rv;
> +  }
> +

2 lines?

::: dom/workers/test/serviceworkers/fetch_event_worker.js
@@ +4,5 @@
> +      var r = new Response("synthesized response body", {});
> +      resolve(r);
> +    });
> +    ev.respondWith(p);
> +  }

can you add a test where the promise doesn't resolve? What should happen in that case?

@@ +38,5 @@
> +      ok &= ev.request.headers.get("X-Test2") == "header2";
> +      var r = new Response(ok.toString(), {});
> +      resolve(r);
> +    });
> +    ev.respondWith(p);    

extra spaces.

::: netwerk/protocol/http/InterceptedChannel.cpp
@@ +90,5 @@
> +}
> +
> +NS_IMETHODIMP
> +InterceptedChannelChrome::GetChannel(nsIChannel** aChannel)
> +{

Should we return NS_OK if mChannel is null?
Attachment #8520821 - Flags: review?(amarchesini) → review+
Comment on attachment 8523967 [details] [diff] [review]
discriminate between navigation and fetch WIP

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

I'm in favour of these changes. Not commenting on the various bits of debug stuff left around on the assumption that those would go away. Want me to fold these into my patch as I'm addressing the review comments?

::: content/base/src/nsDocument.cpp
@@ +3160,5 @@
>          return NS_OK;
>      }
> +
> +    if (aIsNavigate) {
> +      return swm->IsAvailableForURI(aURI, aShouldIntercept);

Would it be worth ignoring this change and just cancelling the interception in ChannelIntercepted instead?

@@ +3170,5 @@
>  NS_IMETHODIMP
>  nsDocument::ChannelIntercepted(nsIInterceptedChannel* aChannel)
>  {
> +    // Ideally this should just be set as a flag on nsIInterceptedChannel, but
> +    // let's try it out first.

Let's do that.

::: dom/workers/ServiceWorkerEvents.cpp
@@ +154,3 @@
>    AutoCancel autoCancel(this);
>  
> +  if (NS_WARN_IF(!aValue.isObject())) {

This can be triggered by web content; our policy is not to spam the console with that.

@@ +158,5 @@
>    }
>  
>    nsRefPtr<Response> response;
>    nsresult rv = UNWRAP_OBJECT(Response, &aValue.toObject(), response);
> +  if (NS_WARN_IF(NS_FAILED(rv))) {

Same as previous.

::: dom/workers/ServiceWorkerManager.cpp
@@ +2216,5 @@
> +    GetServiceWorkerRegistrationInfo(aURI);
> +  *aIsAvailable = !!registration;
> +  if (registration && !registration->mActiveWorker) {
> +    *aIsAvailable = false;
> +  }

*aIsAvailable = registration && registration->mActiveWorker

::: netwerk/protocol/http/HttpBaseChannel.cpp
@@ +1781,5 @@
>    nsCOMPtr<nsINetworkInterceptController> controller;
>    GetCallback(controller);
>    bool shouldIntercept = false;
>    if (controller && !mForceNoIntercept) {
> +    nsresult rv = controller->ShouldPrepareForIntercept(mURI, 

nit: whitespace
Attachment #8523967 - Flags: feedback?(josh) → feedback+
(In reply to Josh Matthews [:jdm] from comment #44)
> Comment on attachment 8523967 [details] [diff] [review]
> discriminate between navigation and fetch WIP
> 
> Review of attachment 8523967 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'm in favour of these changes. Not commenting on the various bits of debug
> stuff left around on the assumption that those would go away. Want me to
> fold these into my patch as I'm addressing the review comments?

Yes, please. Which I assume means you are taking over this patch. If not, let me know :)

> 
> ::: content/base/src/nsDocument.cpp
> @@ +3160,5 @@
> >          return NS_OK;
> >      }
> > +
> > +    if (aIsNavigate) {
> > +      return swm->IsAvailableForURI(aURI, aShouldIntercept);
> 
> Would it be worth ignoring this change and just cancelling the interception
> in ChannelIntercepted instead?

I don't understand what you mean. If we don't have this check, we are querying the wrong serviceworker from this point on.
(In reply to Nikhil Marathe [:nsm] (needinfo? please) from comment #45)
> (In reply to Josh Matthews [:jdm] from comment #44)
> > Comment on attachment 8523967 [details] [diff] [review]
> > discriminate between navigation and fetch WIP
> > 
> > ::: content/base/src/nsDocument.cpp
> > @@ +3160,5 @@
> > >          return NS_OK;
> > >      }
> > > +
> > > +    if (aIsNavigate) {
> > > +      return swm->IsAvailableForURI(aURI, aShouldIntercept);
> > 
> > Would it be worth ignoring this change and just cancelling the interception
> > in ChannelIntercepted instead?
> 
> I don't understand what you mean. If we don't have this check, we are
> querying the wrong serviceworker from this point on.

We could get rid of the aIsNavigate parameter for ShouldIntercept, return true like usual, and then check the navigation property in the ChannelIntercepted method and cancel the interception. It would reduce the number of times we check the channel load flags, since we don't have an nsIInterceptedChannel yet in ShouldIntercept.
Attachment #8520821 - Attachment is obsolete: true
Attachment #8523967 - Attachment is obsolete: true
Attachment #8520819 - Attachment is obsolete: true
Comment on attachment 8534608 [details] [diff] [review]
Dispatch a fetch event to workers when controlled pages initiate a network load

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

::: dom/workers/ServiceWorkerEvents.cpp
@@ +202,5 @@
> +  }
> +
> +  nsCOMPtr<nsIInputStream> body;
> +  response->GetBody(getter_AddRefs(body));
> +  MOZ_ASSERT(body);

This needs to handle the case where the response body is null.  If no body is provided when storing in Cache, then no body will be set when we read it back out.

I think the channel response body should just be closed to indicate that there is no data here, no?
Note, bug 1107516 now creates a load group for the ServiceWorker.  From talking with Josh it sounds like that might impact this bug since the SW load group does not have an nsINetworkInterceptController interface on its callbacks.
I looked a bit more into maple not getting fetch events for all resource loads.  It seems we're HttpBaseChannel::ShouldIntercept() is not finding a controller for the <img> resources.  This could be due to the image channel playing fast and loose with the load group.  See the code here and below:

  http://dxr.mozilla.org/mozilla-central/source/image/src/imgLoader.cpp#656

For example, it might not be propagating the right callback.

It does set the load group eventually as the parent of the channel load group, but it does this after creating the channel.  I tried modifying ShouldIntercept() to look for the parent, but didn't see it.  It seems possible that ShouldIntercept() is running prior to the parent being set.

Josh, can you investigate this issue further?  Thanks!
Flags: needinfo?(josh)
Comment on attachment 8534608 [details] [diff] [review]
Dispatch a fetch event to workers when controlled pages initiate a network load

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

::: dom/base/nsDocument.h
@@ +680,5 @@
>                     public nsIApplicationCacheContainer,
>                     public nsStubMutationObserver,
>                     public nsIObserver,
> +                   public nsIDOMXPathEvaluator,
> +                   public nsINetworkInterceptController

If I understand correctly, you are making the document itself the intercept controller.  It seems, though, this may not work for SharedWorkers.  After bug 1118845 lands, SharedWorkers will always have a load group separate from their originating document.

From talking with Jonas, the fetch event should treat SharedWorkers like an iframe.  The URL of the SharedWorker is used to determine if the scope matches.

So it seems we need to add some kind of intercept controller to the load group in RuntimeService::CreateSharedWorkerFromLoadInfo().

Since SharedWorkers are somewhat rare right now, maybe we can do this as a follow-up?
Not just shared workers, dedicated workers too.
Just a note that upon receiving a Response, step 2.2 of HTTP Fetch should be handled properly.
"If either response's type is opaque and request's mode is not no CORS or response's type is error, return a network error."
No longer blocks: 1120501
This applies on mozilla-central tip and passes all contained mochitests. All interception controller logic has been moved from nsDocument to nsDocShell to account for iframe loads, which do not have a document available and should be treated like a navigation.
Attachment #8534608 - Attachment is obsolete: true
Still no information about why images don't appear to be intercepted on maple. I'm also planning to write additional mochitests to explicitly cover resources loaded from workers and subworkers.
Flags: needinfo?(josh)
Now with tests of fetches originating from workers.
Attachment #8548526 - Attachment is obsolete: true
Attachment #8555902 - Attachment is obsolete: true
Comment on attachment 8555951 [details] [diff] [review]
Dispatch a fetch event to workers when controlled pages initiate a network load

Smaug, could you review the docshell integration? The nsINetworkInterceptionController lived on the Document in previous versions of this patch, but that didn't work for intercepting new iframe loads where there is no document yet.
Attachment #8555951 - Flags: review?(bugs)
Comment on attachment 8555951 [details] [diff] [review]
Dispatch a fetch event to workers when controlled pages initiate a network load

Honza, could you review the new code that extracts HTTP headers from the channel in FetchEventRunnable::Init?
Attachment #8555951 - Flags: review?(honzab.moz)
Comment on attachment 8555951 [details] [diff] [review]
Dispatch a fetch event to workers when controlled pages initiate a network load

Andrea, I believe the rest of the code is basically unchanged from your previous reviews, but I also added many more tests.
Attachment #8555951 - Flags: review?(amarchesini)
Comment on attachment 8555951 [details] [diff] [review]
Dispatch a fetch event to workers when controlled pages initiate a network load

>+nsDocShell::ShouldPrepareForIntercept(nsIURI* aURI, bool aIsNavigate, bool* aShouldIntercept)
>+{
Could you initialize *aShouldIntercept to false before anything else, so that if someone doesn't check
the return value, random *aShouldIntercept isn't returned.

>+  nsCOMPtr<nsIServiceWorkerManager> swm = mozilla::services::GetServiceWorkerManager();
>+  if (!swm) {
>+    *aShouldIntercept = false;
>+    return NS_OK;
>+  }
>+
>+  if (aIsNavigate) {
>+    return swm->IsAvailableForURI(aURI, aShouldIntercept);
>+  }
>+
>+  nsCOMPtr<nsIContentViewer> contentViewer;
>+  GetContentViewer(getter_AddRefs(contentViewer));
>+  if (!contentViewer)
>+    return NS_ERROR_NOT_AVAILABLE;
>+
>+  nsCOMPtr<nsIDocument> doc = contentViewer->GetDocument();
>+  if (!doc)
>+    return NS_ERROR_NOT_AVAILABLE;


Any particular reason you don't just do
nsCOMPtr<nsIDocument> doc = GetDocument();
Or does it matter that we don't create a new content viewer?
If so, 
NS_ENSURE_STATE(mContentViewer); before GetDocument
>+    if (!doc)
>+      return NS_ERROR_NOT_AVAILABLE;
always {} with if.



But it isn't quite clear to me why docshell needs to implement
nsINetworkInterceptController.
I mean, the methods are almost like static methods, which could just take nsIDocShell as param from
which nsIDocument is read if needed. But if adding those methods to docshell makes the implementation easier, fine, and r+ for adding them.


But I can't review ShouldPrepareForIntercept nor ChannelIntercepted since I have no idea what 
aChannel->GetIsNavigation and such mean.
Attachment #8555951 - Flags: review?(bugs)
Comment on attachment 8555951 [details] [diff] [review]
Dispatch a fetch event to workers when controlled pages initiate a network load

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

::: dom/workers/ServiceWorkerManager.cpp
@@ +2055,5 @@
> +  }
> +
> +  NS_DECL_ISUPPORTS
> +      NS_DECL_NSIHTTPHEADERVISITOR
> +      };

indent

@@ +2103,5 @@
> +
> +    rv = uri->GetSpec(mSpec);
> +    NS_ENSURE_SUCCESS(rv, rv);
> +
> +    nsCOMPtr<nsIHttpChannel> httpChannel = do_QueryInterface(channel);

check result first!

@@ +2115,5 @@
> +    //TODO(jdm): we should probably include reload-ness in the loadinfo or as a separate load flag
> +    mIsReload = false;
> +
> +    nsRefPtr<AllRequestHeaders> headerVisitor = new AllRequestHeaders(mHeaderNames, mHeaderValues);
> +    rv = httpChannel->VisitRequestHeaders(headerVisitor);

if not intended for reuse, you could well implement the visitor directly in this class since the lifetime and isolation is per channel I believe, so no need to split further. up to you.
Attachment #8555951 - Flags: review?(honzab.moz) → review+
(In reply to Honza Bambas (:mayhemer) from comment #62)
> Comment on attachment 8555951 [details] [diff] [review]
> Dispatch a fetch event to workers when controlled pages initiate a network
> load
> 
> Review of attachment 8555951 [details] [diff] [review]:

(r+ with comments, only for the header visit parts in ServiceWorkerManager.cpp)
Comment on attachment 8555951 [details] [diff] [review]
Dispatch a fetch event to workers when controlled pages initiate a network load

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

Can you create a test for that AutoJSAPI without document?

::: docshell/base/nsDocShell.cpp
@@ +13816,5 @@
> +  }
> +
> +  nsCOMPtr<nsIContentViewer> contentViewer;
> +  GetContentViewer(getter_AddRefs(contentViewer));
> +  if (!contentViewer)

{}

@@ +13820,5 @@
> +  if (!contentViewer)
> +    return NS_ERROR_NOT_AVAILABLE;
> +
> +  nsCOMPtr<nsIDocument> doc = contentViewer->GetDocument();
> +  if (!doc)

{}

@@ +13844,5 @@
> +
> +  if (!isNavigation) {
> +    nsCOMPtr<nsIContentViewer> contentViewer;
> +    GetContentViewer(getter_AddRefs(contentViewer));
> +    if (!contentViewer)

{}

@@ +13848,5 @@
> +    if (!contentViewer)
> +      return NS_ERROR_NOT_AVAILABLE;
> +
> +    doc = contentViewer->GetDocument();
> +    if (!doc)

{}

::: dom/workers/ServiceWorkerEvents.cpp
@@ +27,5 @@
>  
>  BEGIN_WORKERS_NAMESPACE
>  
> +FetchEvent::FetchEvent(EventTarget* aOwner)
> +: Event(aOwner, nullptr, nullptr)

2 spaces indentation?

@@ +89,5 @@
> +    return NS_OK;
> +  }
> +};
> +
> +class FinishResponse : public nsRunnable

MOZ_FINAL ?

@@ +116,5 @@
> +public:
> +  RespondWithHandler(nsMainThreadPtrHandle<nsIInterceptedChannel>& aChannel,
> +                     nsMainThreadPtrHandle<ServiceWorker>& aServiceWorker)
> +      : mInterceptedChannel(aChannel)
> +      , mServiceWorker(aServiceWorker)

sometimes you indent with 2 spaces and sometimes you don't. Probably we want 2 spaces...

@@ +122,5 @@
> +  }
> +
> +  void
> +      ResolvedCallback(JSContext* aCx, JS::Handle<JS::Value> aValue) MOZ_OVERRIDE;
> +

The indentation here is strange. I suggest:

void Resolvedcallback(JSCOntext* aCx,
                      JS::Handle<JS::Value> aValue) MOZ_OVERRIDE;

@@ +124,5 @@
> +  void
> +      ResolvedCallback(JSContext* aCx, JS::Handle<JS::Value> aValue) MOZ_OVERRIDE;
> +
> +  void
> +      RejectedCallback(JSContext* aCx, JS::Handle<JS::Value> aValue) MOZ_OVERRIDE;

here too.

@@ +134,5 @@
> +{
> +  nsMainThreadPtrHandle<nsIInterceptedChannel> mInterceptedChannel;
> +
> +  RespondWithClosure(nsMainThreadPtrHandle<nsIInterceptedChannel>& aChannel)
> +  : mInterceptedChannel(aChannel)

indentation

@@ +148,5 @@
> +    event = new FinishResponse(data->mInterceptedChannel);
> +  } else {
> +    event = new CancelChannelRunnable(data->mInterceptedChannel);
> +  }
> +  NS_DispatchToMainThread(event);

MOZ_ALWAYS_TRUE(NS_SUCCEEDED(NS_Dispatch...

@@ +157,5 @@
> +  nsRefPtr<RespondWithHandler> mOwner;
> +
> +public:
> +  AutoCancel(RespondWithHandler* aOwner)
> +      : mOwner(aOwner)

too much indentation here :)

@@ +253,5 @@
> +  return client.forget();
> +}
> +
> +already_AddRefed<Promise>
> +FetchEvent::ForwardTo(const nsAString& aUrl)

const ?

@@ +259,5 @@
> +  nsCOMPtr<nsIGlobalObject> global = do_QueryInterface(GetParentObject());
> +  MOZ_ASSERT(global);
> +  ErrorResult result;
> +  nsRefPtr<Promise> promise = Promise::Create(global, result);
> +  if (result.Failed()) {

NS_WARN_IF

@@ +268,5 @@
> +  return promise.forget();
> +}
> +
> +already_AddRefed<Promise>
> +FetchEvent::Default()

this can be const right?

@@ +274,5 @@
> +  nsCOMPtr<nsIGlobalObject> global = do_QueryInterface(GetParentObject());
> +  MOZ_ASSERT(global);
> +  ErrorResult result;
> +  nsRefPtr<Promise> promise = Promise::Create(global, result);
> +  if (result.Failed()) {

NS_WARN_IF

::: dom/workers/ServiceWorkerEvents.h
@@ +6,5 @@
>  #ifndef mozilla_dom_workers_serviceworkerevents_h__
>  #define mozilla_dom_workers_serviceworkerevents_h__
>  
>  #include "mozilla/dom/Event.h"
> +#include "mozilla/dom/FetchEventBinding.h"

alphabetic order.

@@ +26,5 @@
>  class ServiceWorker;
> +class ServiceWorkerClient;
> +
> +class FetchEvent : public Event
> +{

MOZ_FINAL?

@@ +29,5 @@
> +class FetchEvent : public Event
> +{
> +  nsMainThreadPtrHandle<nsIInterceptedChannel> mChannel;
> +  nsMainThreadPtrHandle<ServiceWorker> mServiceWorker;
> +  nsRefPtr<mozilla::dom::workers::ServiceWorkerClient> mClient;

you should not need mozilla::dom::workers...

@@ +30,5 @@
> +{
> +  nsMainThreadPtrHandle<nsIInterceptedChannel> mChannel;
> +  nsMainThreadPtrHandle<ServiceWorker> mServiceWorker;
> +  nsRefPtr<mozilla::dom::workers::ServiceWorkerClient> mClient;
> +  nsRefPtr<mozilla::dom::Request> mRequest;

also mozilla::dom::Request can be replaced by Request, probably.

@@ +59,5 @@
> +              const FetchEventInit& aOptions,
> +              ErrorResult& aRv);
> +
> +  bool
> +  WaitToRespond()

const

@@ +64,5 @@
> +  {
> +    return mWaitToRespond;
> +  }
> +
> +  already_AddRefed<mozilla::dom::Request>

Probably you can change this in:

mozilla::dom::Request
Request() const
{
  return mRequest;
}

@@ +65,5 @@
> +    return mWaitToRespond;
> +  }
> +
> +  already_AddRefed<mozilla::dom::Request>
> +  Request()

const

@@ +75,5 @@
> +  already_AddRefed<mozilla::dom::workers::ServiceWorkerClient>
> +  Client();
> +
> +  bool
> +  IsReload()

const

::: dom/workers/ServiceWorkerManager.cpp
@@ +2049,5 @@
> +  }
> +
> +public:
> +  AllRequestHeaders(nsTArray<nsCString>& aNames, nsTArray<nsCString>& aValues)
> +      : mNames(aNames), mValues(aValues)

what keeps alive these 2 arrays?

@@ +2077,5 @@
> +  nsCString mSpec;
> +  nsCString mMethod;
> +  bool mIsReload;
> + public:
> +  explicit FetchEventRunnable(WorkerPrivate* aWorkerPrivate,

no explicit if more than 1 argument.

@@ +2094,5 @@
> +  Init()
> +  {
> +    nsCOMPtr<nsIChannel> channel;
> +    nsresult rv = mInterceptedChannel->GetChannel(getter_AddRefs(channel));
> +    NS_ENSURE_SUCCESS(rv, rv);

I don't understand if we want NS_ENSURE_SUCCES or if (NS_WARN_IF(NS_FAILED..

@@ +2124,5 @@
> +
> +  bool
> +  WorkerRun(JSContext* aCx, WorkerPrivate* aWorkerPrivate)
> +  {
> +    MOZ_ASSERT(aWorkerPrivate);

MOZ_ASSERT(aCx)

@@ +2133,5 @@
> +  class ResumeRequest MOZ_FINAL : public nsRunnable {
> +    nsMainThreadPtrHandle<nsIInterceptedChannel> mChannel;
> + public:
> +    explicit ResumeRequest(nsMainThreadPtrHandle<nsIInterceptedChannel>& aChannel)
> +        : mChannel(aChannel)

wrong indentation

@@ +2138,5 @@
> +    {
> +    }
> +
> +    NS_IMETHOD
> +        Run()

indentation

@@ +2149,5 @@
> +  };
> +
> +  bool
> +  DispatchFetchEvent(JSContext* aCx, WorkerPrivate* aWorkerPrivate)
> +  {

MOZ_ASSERT aCx and aWorkerPrivate

@@ +2164,5 @@
> +    MOZ_ASSERT(mHeaderNames.Length() == mHeaderValues.Length());
> +    for (uint32_t i = 0; i < mHeaderNames.Length(); i++) {
> +      ErrorResult rv;
> +      internalHeaders->Set(mHeaderNames[i], mHeaderValues[i], rv);
> +      if (rv.Failed()) {

NS_WARN_IF

@@ +2201,5 @@
> +    nsRefPtr<EventTarget> target = do_QueryObject(aWorkerPrivate->GlobalScope());
> +    nsresult rv2 = target->DispatchDOMEvent(nullptr, event, nullptr, nullptr);
> +    if (NS_WARN_IF(NS_FAILED(rv2)) || !event->WaitToRespond()) {
> +      nsCOMPtr<nsIRunnable> runnable = new ResumeRequest(mInterceptedChannel);
> +      NS_DispatchToMainThread(runnable);

MOZ_ALWAYS_TRUE(NS_SUCCEEDED

@@ +2209,5 @@
> +};
> +
> +NS_IMETHODIMP
> +ServiceWorkerManager::DispatchFetchEvent(nsIDocument* aDoc, nsIInterceptedChannel* aChannel)
> +{

MOZ_ASSERT the params?

@@ +2245,5 @@
> +    return rv;
> +  }
> +
> +  nsMainThreadPtrHandle<nsIInterceptedChannel> handle(
> +      new nsMainThreadPtrHolder<nsIInterceptedChannel>(aChannel, false));

wrong indentation

@@ +2248,5 @@
> +  nsMainThreadPtrHandle<nsIInterceptedChannel> handle(
> +      new nsMainThreadPtrHolder<nsIInterceptedChannel>(aChannel, false));
> +
> +  //XXXjdm I think this is ok, since if there's no document there's also no window...
> +  uint64_t windowId = aDoc ? aDoc->GetInnerWindow()->WindowID() : 0;

do you want to file a bug? a follow-up? something to remind it?

@@ +2252,5 @@
> +  uint64_t windowId = aDoc ? aDoc->GetInnerWindow()->WindowID() : 0;
> +
> +  nsRefPtr<ServiceWorker> sw = static_cast<ServiceWorker*>(serviceWorker.get());
> +  nsMainThreadPtrHandle<ServiceWorker> serviceWorkerHandle(
> +      new nsMainThreadPtrHolder<ServiceWorker>(sw));

indentation is 2 spaces

@@ +2255,5 @@
> +  nsMainThreadPtrHandle<ServiceWorker> serviceWorkerHandle(
> +      new nsMainThreadPtrHolder<ServiceWorker>(sw));
> +
> +  nsRefPtr<FetchEventRunnable> event =
> +      new FetchEventRunnable(sw->GetWorkerPrivate(), handle, serviceWorkerHandle, windowId);

indentation is 2 spaces

@@ +2263,5 @@
> +  AutoJSAPI api;
> +  if (aDoc) {
> +    api.Init(aDoc->GetParentObject());
> +  } else {
> +    api.Init();

we cannot do this right? We must enter in a compartment...
Are win workers? In this case just use JSContext from the workerPrivate.

@@ +2274,5 @@
> +}
> +
> +NS_IMETHODIMP
> +ServiceWorkerManager::IsAvailableForURI(nsIURI* aURI, bool* aIsAvailable)
> +{

MOZ_ASSERT aURi and aIsAvailable

@@ +2283,5 @@
> +}
> +
> +NS_IMETHODIMP
> +ServiceWorkerManager::IsControlled(nsIDocument* aDoc, bool* aIsControlled)
> +{

MOZ_ASSERT the params
Attachment #8555951 - Flags: review?(amarchesini) → review+
https://fetch.spec.whatwg.org/#dfnReturnLink-12

This also prevents the Request object having forbidden headers, which are not to be exposed to JS.
>Can you create a test for that AutoJSAPI without document?

That's covered by the iframe test in test_fetch.html.
Assignee: nsm.nikhil → josh
Attachment #8555951 - Attachment is obsolete: true
Attachment #8559433 - Attachment is obsolete: true
Do you think it would be possible to avoid the NS_AsyncCopy() in ServiceWorkerEvents.cpp in the future?  For example, could we allow the Response body to be set as the intercepted channels mSynthesizedInput directly?

It would be a nice optimization as copying from one nsIPipe to another nsIPipe seems a bit wasteful.

Maybe a follow-up?
Comment on attachment 8559925 [details] [diff] [review]
Dispatch a fetch event to workers when controlled pages initiate a network load

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

Sorry for the late feedback, but I think we need to handle the body used flag correctly.  In theory it should be a quick fix.

::: dom/workers/ServiceWorkerEvents.cpp
@@ +191,5 @@
> +  }
> +
> +  nsCOMPtr<nsIInputStream> body;
> +  response->GetBody(getter_AddRefs(body));
> +  MOZ_ASSERT(body);

You need to check BodyUsed() and handle the error if its true.

This should also call SetBodyUsed() after you get the body to prevent the content script from trying to read from the same body stream.

Finally, while rare, I believe its possible for body to be nullptr here.

See steps 9.3.3 here:

  https://slightlyoff.github.io/ServiceWorker/spec/service_worker/index.html#fetch-event-respondwith-method
Attachment #8559925 - Flags: feedback-
Attachment #8559925 - Attachment is obsolete: true
Attached patch fetch_event_fix.patch (obsolete) — — Splinter Review 
I think an error check was lost in the rebase.  I got a nullptr assert when visiting trained-to-thrill without this extra patch.
No longer blocks: 1130803
(In reply to Ben Kelly [:bkelly] from comment #70)
> Do you think it would be possible to avoid the NS_AsyncCopy() in
> ServiceWorkerEvents.cpp in the future?  For example, could we allow the
> Response body to be set as the intercepted channels mSynthesizedInput
> directly?
> 
> It would be a nice optimization as copying from one nsIPipe to another
> nsIPipe seems a bit wasteful.
> 
> Maybe a follow-up?

The problem here is that this is a valid optimization in child processes, but in parent processes all we have is an output stream to a buffer that the HTTP cache provides us with.
Addressed all comments and fixed up the failures from the last try push.
Attachment #8563435 - Attachment is obsolete: true
Attachment #8564471 - Attachment is obsolete: true
Version that fixes a bunch of serious e10s leaks that try exposed.
Attachment #8565497 - Attachment is obsolete: true
Blocks: 1134324
Blocks: 1134325
Blocks: 1134330
Blocks: 1134462
Blocks: 1134465
I did 10 runs on android on try with full logging turned on and got nothing but green. I'm compromising by leaving the `SimpleTest.requestCompleteLog()` in the test and relanding so I'll have something to go on if the test times out on Android again.
https://hg.mozilla.org/mozilla-central/rev/1e12e3b68478
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox38: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
Josh, should the call to GetDocumentController() be passed the doc's inner window? There are certain DETH assertions being hit in M7 here - https://treeherder.mozilla.org/logviewer.html#?job_id=5123802&repo=try
Flags: needinfo?(josh)
Blocks: 1136281
Blocks: 1136757
Looks like yes, but that's being addressed in another bug.
Flags: needinfo?(josh)
Blocks: 1137250
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: