Calling GetClient() on on FetchEvent asserts when there's no document (i.e. when intercepting the document's initial load).

Hi Cătălin, I would like to work on this bug, could you help me on this? thanks!

Hey, So the assert is hit when loading https://catalinb.github.io/swpostmessage using a debug build. Here's the stack trace: rame #0: 0x0000000104b308e9 XUL`mozilla::dom::workers::ServiceWorkerClient::ServiceWorkerClient(this=0x0000000124c9cf20, aOwner=0x0000000000000000, aClientInfo=0x000000012bd99180) + 281 at ServiceWorkerClient.h:59 56 , mUrl(aClientInfo.mUrl) 57 , mWindowId(aClientInfo.mWindowId) 58 { -> 59 MOZ_ASSERT(aOwner); 60 } 61 62 nsISupports* (lldb) bt * thread #62: tid = 0x7b7e, 0x0000000104b308e9 XUL`mozilla::dom::workers::ServiceWorkerClient::ServiceWorkerClient(this=0x0000000124c9cf20, aOwner=0x0000000000000000, aClientInfo=0x000000012bd99180) + 281 at ServiceWorkerClient.h:59, name = 'DOM Worker', stop reason = EXC_BAD_ACCESS (code=1, address=0x0) * frame #0: 0x0000000104b308e9 XUL`mozilla::dom::workers::ServiceWorkerClient::ServiceWorkerClient(this=0x0000000124c9cf20, aOwner=0x0000000000000000, aClientInfo=0x000000012bd99180) + 281 at ServiceWorkerClient.h:59 frame #1: 0x0000000104b8eea5 XUL`mozilla::dom::workers::ServiceWorkerClient::ServiceWorkerClient(this=0x0000000124c9cf20, aOwner=0x0000000000000000, aClientInfo=0x000000012bd99180) + 37 at ServiceWorkerClient.h:60 frame #2: 0x0000000104b38341 XUL`mozilla::dom::workers::FetchEvent::GetClient(this=0x0000000129861110) + 209 at ServiceWorkerEvents.cpp:342 frame #3: 0x0000000103d93578 XUL`mozilla::dom::FetchEventBinding::get_client(cx=0x000000012ea41d60, obj=Handle<JSObject *> at 0x000000013c4f9be0, self=0x0000000129861110, args=JSJitGetterCallArgs at 0x000000013c4f9bd8) + 40 at FetchEventBinding.cpp:516 frame #4: 0x0000000103fbadbb XUL`mozilla::dom::GenericBindingGetter(cx=0x000000012ea41d60, argc=0, vp=0x000000013c4fa748) + 651 at BindingUtils.cpp:2539 frame #5: 0x0000000106fbc568 XUL`js::CallJSNative(cx=0x000000012ea41d60, native=0x0000000103fbab30, args=0x000000013c4fa5e0)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) + 184 at jscntxtinlines.h:235 frame #6: 0x0000000106f4f89d XUL`js::Invoke(cx=0x000000012ea41d60, args=CallArgs at 0x000000013c4fa5e0, construct=NO_CONSTRUCT) + 1245 at Interpreter.cpp:727 frame #7: 0x0000000106f372be XUL`js::Invoke(cx=0x000000012ea41d60, thisv=0x000000013c4fa7c8, fval=0x000000013c4fa7f0, argc=0, argv=0x0000000000000000, rval=JS::MutableHandleValue at 0x000000013c4fa6e0) + 894 at Interpreter.cpp:783 frame #8: 0x0000000106f769f0 XUL`js::InvokeGetter(cx=0x000000012ea41d60, obj=0x000000013c70f1c0, fval=Value at 0x000000013c4fa7f0, rval=JS::MutableHandleValue at 0x000000013c4fa7e8) + 160 at Interpreter.cpp:852 frame #9: 0x000000010709ee26 XUL`CallGetter(cx=0x000000012ea41d60, obj=JS::HandleObject at 0x000000013c4fa8c0, receiver=JS::HandleObject at 0x000000013c4fa8b8, shape=js::HandleShape at 0x000000013c4fa8b0, vp=JS::MutableHandleValue at 0x000000013c4fa8a8) + 230 at NativeObject.cpp:1610 frame #10: 0x000000010705ebc7 XUL`bool GetExistingProperty<(js::AllowGC)1>(cx=0x000000012ea41d60, receiver=js::MaybeRooted<JSObject *, js::AllowGC>::HandleType at 0x000000013c4fa9c0, obj=js::MaybeRooted<js::NativeObject *, js::AllowGC>::HandleType at 0x000000013c4fa9b8, shape=js::MaybeRooted<js::Shape *, js::AllowGC>::HandleType at 0x000000013c4fa9b0, vp=js::MaybeRooted<JS::Value, js::AllowGC>::MutableHandleType at 0x000000013c4fa9a8)1>::HandleType, js::MaybeRooted<js::NativeObject*, (js::AllowGC)1>::HandleType, js::MaybeRooted<js::Shape*, (js::AllowGC)1>::HandleType, js::MaybeRooted<JS::Value, (js::AllowGC)1>::MutableHandleType) + 855 at NativeObject.cpp:1658 frame #11: 0x000000010705ef71 XUL`bool NativeGetPropertyInline<(js::AllowGC)1>(cx=0x000000012ea41d60, obj=js::MaybeRooted<js::NativeObject *, js::AllowGC>::HandleType at 0x000000013c4fab80, receiver=js::MaybeRooted<JSObject *, js::AllowGC>::HandleType at 0x000000013c4fab78, id=js::MaybeRooted<jsid, js::AllowGC>::HandleType at 0x000000013c4fab70, nameLookup=NotNameLookup, vp=js::MaybeRooted<JS::Value, js::AllowGC>::MutableHandleType at 0x000000013c4fab68)1>::HandleType, js::MaybeRooted<JSObject*, (js::AllowGC)1>::HandleType, js::MaybeRooted<jsid, (js::AllowGC)1>::HandleType, IsNameLookup, js::MaybeRooted<JS::Value, (js::AllowGC)1>::MutableHandleType) + 561 at NativeObject.cpp:1876 frame #12: 0x000000010705ed2a XUL`js::NativeGetProperty(cx=0x000000012ea41d60, obj=js::HandleNativeObject at 0x000000013c4fabe8, receiver=JS::HandleObject at 0x000000013c4fabe0, id=JS::HandleId at 0x000000013c4fabd8, vp=JS::MutableHandleValue at 0x000000013c4fabd0) + 90 at NativeObject.cpp:1910 frame #13: 0x0000000106fb1326 XUL`js::GetProperty(cx=0x000000012ea41d60, obj=JS::HandleObject at 0x000000013c4fac70, receiver=JS::HandleObject at 0x000000013c4fac68, id=JS::HandleId at 0x000000013c4fac60, vp=JS::MutableHandleValue at 0x000000013c4fac58) + 214 at NativeObject.h:1411 frame #14: 0x0000000106f8a053 XUL`GetPropertyOperation(cx=0x000000012ea41d60, fp=0x000000012ea23020, script=JS::HandleScript at 0x000000013c4fae70, pc=0x000000012e739d1b, lval=JS::MutableHandleValue at 0x000000013c4fae68, vp=JS::MutableHandleValue at 0x000000013c4fae60) + 1267 at Interpreter.cpp:270 frame #15: 0x0000000106f6791b XUL`Interpret(cx=0x000000012ea41d60, state=0x000000013c4fe180) + 43195 at Interpreter.cpp:2675 frame #16: 0x0000000106f5cf6b XUL`js::RunScript(cx=0x000000012ea41d60, state=0x000000013c4fe180) + 715 at Interpreter.cpp:677 frame #17: 0x0000000106f4f9db XUL`js::Invoke(cx=0x000000012ea41d60, args=CallArgs at 0x000000013c4fe980, construct=NO_CONSTRUCT) + 1563 at Interpreter.cpp:746 frame #18: 0x0000000106f372be XUL`js::Invoke(cx=0x000000012ea41d60, thisv=0x000000013c4fee70, fval=0x000000013c4fecc8, argc=1, argv=0x000000013c4fed98, rval=JS::MutableHandleValue at 0x000000013c4fea80) + 894 at Interpreter.cpp:783

the fetch handler from sw.js tries to access .client, for some reason the client has null owner and the assert is hit. You'll need to identify which fetch event causes this and why we don't have a owner for the client object. See ServiceWorkerManager::DispatchFetchEvent.

Hi Cătălin, Thanks! I will work on this immediately after finishing my current work.

I'll start to work on this issue

Hi Cătălin, Following is my trace result, please correct me if there is anything wrong :) |mOwner| object of FetchEvent should be set in Event::SetOwner when we new FetchEvent(owner) https://dxr.mozilla.org/mozilla-central/source/dom/events/Event.cpp#1166 https://dxr.mozilla.org/mozilla-central/source/dom/workers/ServiceWorkerEvents.cpp#64 in this case, it should be done by following code in Event::SetOwner nsCOMPtr<DOMEventTargetHelper> eth = do_QueryInterface(aOwner); if (eth) { mOwner = eth->GetParentObject(); return; } I found eth->GetParentObject() is null, so it means |mParentObject| of DOMEventTargetHelper is null. |mParentObject| should be set in BindToOwner function inside DOMEventTargetHelper constructor. But if DOMEventTargetHelper constructor is called with default constructor, then BindToOwner function will not be called https://dxr.mozilla.org/mozilla-central/source/dom/events/DOMEventTargetHelper.h#35 I check when do we create this |DOMEventTargetHelper|, and found it is created in WorkerGlobalScope because WorkerGlobalScope inherit DOMEventTargetHelper https://dxr.mozilla.org/mozilla-central/source/dom/workers/WorkerScope.cpp?from=WorkerScope.cpp#54 So default constructor of DOMEventTargetHelper is called without setting |mParentObject| Maybe this is the reason |mOwner| is null ? this is the stack we create global object #0 mozilla::DOMEventTargetHelper::DOMEventTargetHelper (this=0x7fa53ca6f230) at ../../dist/include/mozilla/DOMEventTargetHelper.h:35 #1 0x00007fa558eb35f4 in mozilla::dom::workers::WorkerGlobalScope::WorkerGlobalScope (this=0x7fa53ca6f230, aWorkerPrivate=0x7fa53ca75000) at /home/dimi/Project/SW/mozilla-central/dom/workers/WorkerScope.cpp:55 #2 0x00007fa558eb5117 in mozilla::dom::workers::ServiceWorkerGlobalScope::ServiceWorkerGlobalScope ( this=0x7fa53ca6f230, aWorkerPrivate=0x7fa53ca75000, aScope=...) at /home/dimi/Project/SW/mozilla-central/dom/workers/WorkerScope.cpp:446 #3 0x00007fa558eb0a9e in mozilla::dom::workers::WorkerPrivate::GetOrCreateGlobalScope (this=0x7fa53ca75000, aCx=0x7fa53cdfb920) at /home/dimi/Project/SW/mozilla-central/dom/workers/WorkerPrivate.cpp:7155 #4 0x00007fa558e9fdd3 in (anonymous namespace)::CompileScriptRunnable::WorkerRun (this=0x7fa53caf95c0, aCx=0x7fa53cdfb920, aWorkerPrivate=0x7fa53ca75000) at /home/dimi/Project/SW/mozilla-central/dom/workers/WorkerPrivate.cpp:1049 #5 0x00007fa558eb267a in mozilla::dom::workers::WorkerRunnable::Run (this=0x7fa53caf95c0) at /home/dimi/Project/SW/mozilla-central/dom/workers/WorkerRunnable.cpp:357

Hi Cătălin, Do you have any suggestion or comment about this ? Thanks for help !

It looks like any event dispatched on the worker global scope object will have a null parent, which is a bug.

Hi Cătălin, So in this case, who should be the parent of |ServiceWorkerGlobalScope| ? Do you have any suggestion how to fix this issue ? Thanks for help!

As discussed with Cătălin in IRC, this issue is not about null parent of |ServiceWorkerGlobalScope|. The problem is that SetOwner should set correct owner when dispatching messages to the global scope

I am not sure if i should fix in FetchEvent::GetClient or Event::SetOwner But I see https://dxr.mozilla.org/mozilla-central/source/dom/events/Event.cpp#67 If update SetOwner to make |mOwner| non-null, then in this case |mIsMainThreadEvent| will become true which i think might be wrong in our scenario.

Comment on attachment 8611108 [details] [diff] [review] Patch - Fix in FetchEvent Yes, this the correct fix. Could you please also add a simple mochitest in which you test the client property of the fetch event. Something in the following lines: 1. register a service worker 2. claim the test page 3. make a dummy xhr 4. intercept the fetch event and use the client to post a message 5. check the test page received the message. Thanks!

Hi Cătălin, I tried writing a mochitest, client.postMessage will be executed, but window will not receive message ? Could you help check what am i doing wrong in this patch ? thanks!

Comment on attachment 8613450 [details] [diff] [review] mochitest.patch Review of attachment 8613450 [details] [diff] [review]: ----------------------------------------------------------------- Sorry for the delay. Could you please confirm that event.client.url matches dummy.html in the fetch event handler? ::: dom/workers/test/serviceworkers/test_fetch_event_client.html @@ +35,5 @@ > + function testFetchEvent() { > + dump("[Dimi]testFetchEvent\n"); > + var p = new Promise(function(resolve, reject) { > + var w = window.open("dummy.html"); > + w.onmessage = function(msg) { Use navigator.serviceWorker.onmessage for messages dispatched by service workers through the client interface. @@ +53,5 @@ > + SimpleTest.finish(); > + }).catch(function(e) { > + ok(false, "Some test failed with error " + e); > + SimpleTest.finish(); > + }); instead of the double SimpleTest.finish() you can do .then(SimpleTest.finish) after the catch block.

Hi Cătălin, The event.client.url is "http://mochi.test:8888/tests/dom/workers/test/serviceworkers/dummy.html" And dummy.html is put in dom/workers/test/serviceworkers/dummy.html I change |w.onmessage| to |navigator.serviceWorker.onmessage|, but still cannot receive the message. Do you have any suggestion ?

(In reply to Dimi Lee[PTO until 6/22][:dimi][:dlee] from comment #15) > Hi Cătălin, > The event.client.url is > "http://mochi.test:8888/tests/dom/workers/test/serviceworkers/dummy.html" > And dummy.html is put in dom/workers/test/serviceworkers/dummy.html > > I change |w.onmessage| to |navigator.serviceWorker.onmessage|, but still > cannot receive the message. > Do you have any suggestion ? Try w.navigator.serviceWorker.onmessage.

(In reply to Cătălin Badea (:catalinb) from comment #16) > (In reply to Dimi Lee[PTO until 6/22][:dimi][:dlee] from comment #15) > > Hi Cătălin, > > The event.client.url is > > "http://mochi.test:8888/tests/dom/workers/test/serviceworkers/dummy.html" > > And dummy.html is put in dom/workers/test/serviceworkers/dummy.html > > > > I change |w.onmessage| to |navigator.serviceWorker.onmessage|, but still > > cannot receive the message. > > Do you have any suggestion ? > > Try w.navigator.serviceWorker.onmessage. Hi Cătălin, Yes, it works! I will update another patch later, thanks for help !

Comment on attachment 8628134 [details] [diff] [review] mochitest patch v2 Review of attachment 8628134 [details] [diff] [review]: ----------------------------------------------------------------- The test is ok. I don't really like that the dummy window is left open and that dummy.html generates the fetch events through the script and css links. ::: dom/workers/test/serviceworkers/dummy.html @@ +1,1 @@ > +<!-- Move this file to a sub-directory, sw_clients/ for example. @@ +5,5 @@ > +<!DOCTYPE HTML> > +<html> > +<head> > + <title>Bug 1158735 - Dummy page</title> > + <script type="text/javascript" src="/tests/SimpleTest/SimpleTest.js"> Remove this. @@ +7,5 @@ > +<head> > + <title>Bug 1158735 - Dummy page</title> > + <script type="text/javascript" src="/tests/SimpleTest/SimpleTest.js"> > + </script> > + <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css" /> Remove this. The fetch events intercepted by the service workers are generated by the script and link tags. How about we use something more explicit: window.onload = function() { navigator.serviceWorker.ready.then(function(swr) { fetch("foobar.txt"); }); } ::: dom/workers/test/serviceworkers/test_fetch_event_client_postmessage.html @@ +31,5 @@ > + } > + > + function testFetchEvent() { > + var p = new Promise(function(resolve, reject) { > + var w = window.open("dummy.html"); This window is never closed. It would be better to use an iframe instead, you can access the iframe's window using iframe.contentWindow. @@ +34,5 @@ > + var p = new Promise(function(resolve, reject) { > + var w = window.open("dummy.html"); > + > + w.navigator.serviceWorker.onmessage = function(msg) { > + ok(true, "test ok"); check the message that was received is the correct one. nit: maybe a more descriptive message?

Hi Cătălin, In this patch i use iframe and also call fetch when window is loaded. service worker receives "fetch" event twice (one is for dummy.html another is because of calling fetch in dummy.html) The following debug message is printed and could you help check if it is reasonable ? thanks ! [Dimi]open dummy.html [Dimi]event.client.url = http://mochi.test:8888/tests/dom/workers/test/serviceworkers/sw_clients/dummy.html [Dimi]onmessage [object MessageEvent] [Dimi]origin = // both origin and source is empty, is it correct ? [Dimi]source = null [Dimi]dummy page is loaded [Dimi]fetch foo.txt [Dimi]event.client.url = http://mochi.test:8888/tests/dom/workers/test/serviceworkers/sw_clients/dummy.html // the url is still dummy.html, not "foo.txt" in this case ? [Dimi]onmessage [object MessageEvent] [Dimi]origin = // Again, both origin and source is empty, is it correct ? [Dimi]source = null

Comment on attachment 8631478 [details] [diff] [review] mochitest patch v3 Review of attachment 8631478 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/workers/test/serviceworkers/sw_clients/dummy.html @@ +5,5 @@ > +<!DOCTYPE HTML> > +<html> > +<head> > + <title>Bug 1158735 - Dummy page</title> > + <script type="text/javascript" src="/tests/SimpleTest/SimpleTest.js"></script> Remove this line. This triggers the extra fetch event.

(In reply to Dimi Lee[:dimi][:dlee] from comment #20) > Created attachment 8631478 [details] [diff] [review] > mochitest patch v3 > > Hi Cătălin, > In this patch i use iframe and also call fetch when window is loaded. > service worker receives "fetch" event twice (one is for dummy.html another > is because of calling fetch in dummy.html) > > The following debug message is printed and could you help check if it is > reasonable ? thanks ! > > [Dimi]open dummy.html > [Dimi]event.client.url = > http://mochi.test:8888/tests/dom/workers/test/serviceworkers/sw_clients/ > dummy.html > [Dimi]onmessage [object MessageEvent] > [Dimi]origin = // both origin and source is empty, is it correct > ? > [Dimi]source = null > [Dimi]dummy page is loaded > [Dimi]fetch foo.txt > [Dimi]event.client.url = > http://mochi.test:8888/tests/dom/workers/test/serviceworkers/sw_clients/ > dummy.html // the url is still dummy.html, not "foo.txt" in this case ? You need to check event.request.url to see the url that's being fetched. event.client.url == dummy is correct in this instance since dummy.html is the "service worker client" whose network requests are being intercepted. > [Dimi]onmessage [object MessageEvent] > [Dimi]origin = // Again, both origin and source is empty, is it correct > ? > [Dimi]source = null These fields are not set when doing a ServiceWorkerClient::PostMessage at the moment. The spec now includes service worker specific message event interfaces. This should be fixed once these interfaces (ServiceWorkerMessageEvent, ExtendableMessageEvent) are implemented in gecko.

Address catalin's comment

Comment on attachment 8634656 [details] [diff] [review] mochitest patch v4 Review of attachment 8634656 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. ::: dom/workers/test/serviceworkers/fetch_event_client.js @@ +1,1 @@ > +var INTERCEPT_URL = nit: rename this to CLIENT_URL since it is not the intercepted url, but the url of the window whose fetch is being intercepted. Again, if you want to check the url of the network request that was intercepted use event.request.url.

(In reply to Cătălin Badea (:catalinb) from comment #24) > Comment on attachment 8634656 [details] [diff] [review] > mochitest patch v4 > > Review of attachment 8634656 [details] [diff] [review]: > ----------------------------------------------------------------- > > Looks good. > > ::: dom/workers/test/serviceworkers/fetch_event_client.js > @@ +1,1 @@ > > +var INTERCEPT_URL = > > nit: rename this to CLIENT_URL since it is not the intercepted url, but the > url of the window whose fetch is being intercepted. > > Again, if you want to check the url of the network request that was > intercepted use event.request.url. Ah, Ok, Thanks for the information!

Address catalin's comment

Comment on attachment 8611108 [details] [diff] [review] Patch - Fix in FetchEvent Review of attachment 8611108 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/workers/ServiceWorkerEvents.cpp @@ +343,5 @@ > + WorkerPrivate* worker = GetCurrentThreadWorkerPrivate(); > + MOZ_ASSERT(worker); > + nsRefPtr<nsIGlobalObject> global = worker->GlobalScope(); > + > + mClient = new ServiceWorkerClient(global, *mClientInfo); Can you explain how GetParentObject() doesn't currently return the worker global? It seems its being passed in to the FetchEvent::Constructor() method as the owner (using a GlobalObject stack wrapper).

Comment on attachment 8611108 [details] [diff] [review] Patch - Fix in FetchEvent Review of attachment 8611108 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/workers/ServiceWorkerEvents.cpp @@ +343,5 @@ > + WorkerPrivate* worker = GetCurrentThreadWorkerPrivate(); > + MOZ_ASSERT(worker); > + nsRefPtr<nsIGlobalObject> global = worker->GlobalScope(); > + > + mClient = new ServiceWorkerClient(global, *mClientInfo); Actually, the owner is explicitly ignored in Event::SetOwner() unless its a particular type: https://dxr.mozilla.org/mozilla-central/source/dom/events/Event.cpp#1180 So this looks reasonable to me.

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a7ec062106c5

rebase to latest code and run try