1156397 - Small bugs in webpush PushService

Status: RESOLVED FIXED

(Core :: DOM: Push Subscriptions, defect)

Description

[Dragana Damjanovic [:dragana]]

The line:
http://mxr.mozilla.org/mozilla-central/source/dom/push/PushService.jsm#385

does not look wright. Should be record

I will use this bug to fix if i find other problems.

Comment 1

[Dragana Damjanovic [:dragana]]

Attachment: bug_1156397_v1.patch

Comment 2

[Dragana Damjanovic [:dragana]]

Call:
http://mxr.mozilla.org/mozilla-central/source/dom/workers/ServiceWorkerManager.cpp#1543

does not work because the first argument cannot be nullptr it must be nsIDocument.

Suggestion from:
https://bugzilla.mozilla.org/show_bug.cgi?id=1038811#c64
 target->DispatchTrustedEvent(NS_LITERAL_STRING("..."));

does not work because  DOMEventTargetHelper is not thread safe....

I am not familiar with this code, I will leave the ExtendableEvent for now

Comment 3

[Doug Turner (:dougt)]

Comment on attachment 8595444 [details] [diff] [review]
bug_1156397_v1.patch

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

nsm, can you look at the swm changes?

::: dom/interfaces/base/nsIServiceWorkerManager.idl
@@ -122,5 @@
>    // of nsIServiceWorkerInfo.
>    nsIArray getAllRegistrations();
>  
>    void sendPushEvent(in ACString scope, in DOMString data);
> -  void sendPushSubscriptionChangedEvent(in ACString scope);

update the uuid of this interface.

::: dom/push/PushService.jsm
@@ +1330,2 @@
>  
> +        // TODO -- test needed.  E10s support needed.

while you're here, you can drop the "E10s support needed." of this comment.  It should work in e10s.

Comment 4

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Comment on attachment 8595444 [details] [diff] [review]
bug_1156397_v1.patch

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

::: dom/workers/ServiceWorkerManager.cpp
@@ +1536,5 @@
>    WorkerRun(JSContext* aCx, WorkerPrivate* aWorkerPrivate) override
>    {
>      MOZ_ASSERT(aWorkerPrivate);
> +    GlobalObject globalObj(aCx, aWorkerPrivate->GlobalScope()->GetWrapper());
> +    nsCOMPtr<EventTarget> owner = do_QueryInterface(globalObj.GetAsSupports());

If the intent is to capture https://github.com/w3c/push-api/issues/132, please add a link to the issue, since the spec still keeps this a simple event. Also file a followup for implementing the part where we respect waitUntil() and put the bug number here. Thanks!

There is some duplication here, and globalObj is not used. How about:
nsRefPtr<EventTarget> target = aWorkerPrivate->GlobalScope();
and pass target to ExtendableEvent::Constructor and also use it below for DispatchDOMEvent.

@@ +1548,2 @@
>  
>      nsRefPtr<EventTarget> target = do_QueryObject(aWorkerPrivate->GlobalScope());

Not required.

Comment 5

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: interdiff

Comment 6

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Comment on attachment 8596206 [details] [diff] [review]
interdiff

sorry, wrong bug.

Comment 7

[Dragana Damjanovic [:dragana]]

(In reply to Nikhil Marathe [:nsm] (needinfo? please) from comment #4)
> Comment on attachment 8595444 [details] [diff] [review]
> bug_1156397_v1.patch
> 
> Review of attachment 8595444 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/workers/ServiceWorkerManager.cpp
> @@ +1536,5 @@
> >    WorkerRun(JSContext* aCx, WorkerPrivate* aWorkerPrivate) override
> >    {
> >      MOZ_ASSERT(aWorkerPrivate);
> > +    GlobalObject globalObj(aCx, aWorkerPrivate->GlobalScope()->GetWrapper());
> > +    nsCOMPtr<EventTarget> owner = do_QueryInterface(globalObj.GetAsSupports());
> 
> If the intent is to capture https://github.com/w3c/push-api/issues/132,
> please add a link to the issue, since the spec still keeps this a simple
> event. Also file a followup for implementing the part where we respect
> waitUntil() and put the bug number here. Thanks!


Actually that was not my intention, I just did not know how to do it properly, because I am very new to dom code.
But I have figured it out.

Comment 8

[Dragana Damjanovic [:dragana]]

Attachment: bug_1156397_v1.patch

Comment 9

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Comment on attachment 8596658 [details] [diff] [review]
bug_1156397_v1.patch

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

r=me with comments once you explain or remove the Throw() call.

::: dom/workers/ServiceWorkerManager.cpp
@@ +1537,5 @@
>    {
>      MOZ_ASSERT(aWorkerPrivate);
>  
> +    WorkerGlobalScope* globalScope = aWorkerPrivate->GlobalScope();
> +    

Nit: Trailing whitespace.

@@ +1541,5 @@
> +    
> +    nsCOMPtr<nsIDOMEvent> event;
> +    nsresult rv =
> +      NS_NewDOMEvent(getter_AddRefs(event), globalScope, nullptr, nullptr);
> +    if (NS_FAILED(rv)) {

if (NS_WARN_IF(NS_FAILED(rv)) {

@@ +1542,5 @@
> +    nsCOMPtr<nsIDOMEvent> event;
> +    nsresult rv =
> +      NS_NewDOMEvent(getter_AddRefs(event), globalScope, nullptr, nullptr);
> +    if (NS_FAILED(rv)) {
> +      Throw(aCx, rv);

See comment below about throw.

@@ +1547,5 @@
> +      return false;
> +    }
> +
> +    rv = event->InitEvent(NS_LITERAL_STRING("pushsubscriptionchange"), false, false);
> +    if (NS_FAILED(rv)) {

if (NS_WARN_IF(NS_FAILED(rv)) {

@@ +1548,5 @@
> +    }
> +
> +    rv = event->InitEvent(NS_LITERAL_STRING("pushsubscriptionchange"), false, false);
> +    if (NS_FAILED(rv)) {
> +      Throw(aCx, rv);

Where does this Throw() function come from and what does it do? If there was a failure, just returning is fine.

Comment 10

[Dragana Damjanovic [:dragana]]

Attachment: bug_1156397_v1.patch

Comment 11

[Dragana Damjanovic [:dragana]]

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

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5fa88d413c4f

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5fa88d413c4f