1275363 - Fix ext-notifications.js, line 59: TypeError: this is undefined

Status: RESOLVED FIXED

(WebExtensions :: Untriaged, defect, P2)

Description

[Bob Silverberg [:bsilverberg]]

The tests are logging JavaScript errors (e.g., [1]) indicating that the code at [2] needs to be fixed.

[1] http://archive.mozilla.org/pub/firefox/tinderbox-builds/fx-team-macosx64-debug/1464100633/fx-team_yosemite_r7-debug_test-mochitest-e10s-5-bm108-tests1-macosx-build16.txt.gz
[2] https://dxr.mozilla.org/mozilla-central/source/toolkit/components/extensions/ext-notifications.js#59

Comment 1

[Bob Silverberg [:bsilverberg]]

Attachment: MozReview Request: Bug 1275363 - Fix ext-notifications.js, line 59: TypeError: this is undefined, r?kmag

Review commit: https://reviewboard.mozilla.org/r/54926/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/54926/

Comment 2

[Bob Silverberg [:bsilverberg]]

https://reviewboard.mozilla.org/r/54926/#review51570

::: toolkit/components/extensions/ext-notifications.js:57
(Diff revision 1)
>    },
>  
>    observe(subject, topic, data) {
>      let notifications = notificationsMap.get(this.extension);
>  
> -    function emitAndDelete(event) {
> +    let emitAndDelete = event => {

I'm not sure how to write a test for this as the only event we can write an automated test for is
`onClosed`, but onClosed can only be invoked by calling `notifications.clear()`, which itself deletes the notification, so checking to see if the notification is in fact deleted after onClosed fires wouldn't prove anything.

Do you have any suggestions for how to write a test to prove this is fixed?

Comment 3

[Bob Silverberg [:bsilverberg]]

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

Comment 4

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8756012 [details]
MozReview Request: Bug 1275363 - Fix ext-notifications.js, line 59: TypeError: this is undefined, r?kmag

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54926/diff/1-2/

Comment 5

[Bob Silverberg [:bsilverberg]]

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

Comment 6

[Kris Maglione [:kmag]]

https://reviewboard.mozilla.org/r/54926/#review51570

> I'm not sure how to write a test for this as the only event we can write an automated test for is
> `onClosed`, but onClosed can only be invoked by calling `notifications.clear()`, which itself deletes the notification, so checking to see if the notification is in fact deleted after onClosed fires wouldn't prove anything.
> 
> Do you have any suggestions for how to write a test to prove this is fixed?

Check that the ID has been removed from the notification map after the event is emitted.

Comment 7

[Bob Silverberg [:bsilverberg]]

https://reviewboard.mozilla.org/r/54926/#review51570

> Check that the ID has been removed from the notification map after the event is emitted.

I'm not sure what you are suggesting. Is this something to add into `ext-notifications.js`, like an assert (which doesn't seem right to me), or something to add to the test? The latter makes more sense, but I still don't see how to do it. I cannot interrogate the actual contents of the notifications map from a test, but I can use `notifications.get` to see what's currently in there. The problem remains that the only way I can trigger an event (in an automated test) is to call `notifications.clear`, and that itself also removes the notification from the map, so checking the results from `notifications.get` inside the listener always shows the notification as being removed, even without this bug fix.

Comment 8

[Kris Maglione [:kmag]]

You can check the actual contents of the map from the mochitest scope. But the fact that we don't currently have a way to trigger the events that cause this is another issue, so let's leave it aside for now.

We should probably add an interface to mock those events in the future, so can you please file a follow-up for that?

Comment 9

[Kris Maglione [:kmag]]

Comment on attachment 8756012 [details]
MozReview Request: Bug 1275363 - Fix ext-notifications.js, line 59: TypeError: this is undefined, r?kmag

https://reviewboard.mozilla.org/r/54926/#review52270

Comment 10

[Bob Silverberg [:bsilverberg]]

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

Comment 11

[Bob Silverberg [:bsilverberg]]

(In reply to Kris Maglione [:kmag] from comment #8)
> 
> We should probably add an interface to mock those events in the future, so
> can you please file a follow-up for that?

I opened https://bugzilla.mozilla.org/show_bug.cgi?id=1276214

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/f59918cf04fa

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f59918cf04fa

Comment 14

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8756012 [details]
MozReview Request: Bug 1275363 - Fix ext-notifications.js, line 59: TypeError: this is undefined, r?kmag

Approval Request Comment
[Feature/regressing bug #]: This fixes a bug introduced in Bug 1265797, for which uplift is also being requested.
[User impact if declined]: This is a bug in the WebExtensions notifications API which is meant to be feature-complete in 48. Users of the API could easily hit this bug.
[Describe test coverage new/current, TreeHerder]: Unfortunately it was not possible to write an automated test for this change, but it was tested manually.
[Risks and why]: This is a very simple change, isolated to toolkit/components/extensions/ext-notifications.js so can be considered to be low risk. 
[String/UUID change made/needed]:none

Note also that this is a follow-up to bug 1265797 for which uplift is also being requested. Both patches will need to be uplifted.

Comment 15

[Bob Silverberg [:bsilverberg]]

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

Comment 16

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8756012 [details]
MozReview Request: Bug 1275363 - Fix ext-notifications.js, line 59: TypeError: this is undefined, r?kmag

Improve WebExtension, taking it

Comment 17

[Carsten Book [:Tomcat]]

has problems to apply to aurora:

grafting 347143:f59918cf04fa "Bug 1275363 - Fix ext-notifications.js, line 59: TypeError: this is undefined, r=kmag"
merging toolkit/components/extensions/ext-notifications.js
warning: conflicts while merging toolkit/components/extensions/ext-notifications.js! (edit, then use 'hg resolve --mark')
abort: unresolved conflicts, can't continue
(use 'hg resolve' and 'hg graft --continue')

Comment 18

[Bob Silverberg [:bsilverberg]]

Thanks Carsten. That's odd, as I was able to apply each patch to aurora locally when I tested. I'll try again now, with the other two landed.

Comment 19

[Bob Silverberg [:bsilverberg]]

I'm confused. Maybe there's something wrong with my local aurora branch, but the commit seems to apply fine for me. Here's what I did:
from /mozilla-central:
hg up aurora
hg pull -u aurora
hg graft f59918cf04fa

It applies fine with no errors.

Do you know what I might need to do to resolve the issue?

Comment 20

[Bob Silverberg [:bsilverberg]]

Attachment: Patch to use for uplifting to aurora

Approval Request Comment
[Feature/regressing bug #]: This fixes a bug introduced in Bug 1265797, for which uplift is also being requested.
[User impact if declined]: This is a bug in the WebExtensions notifications API which is meant to be feature-complete in 48. Users of the API could easily hit this bug.
[Describe test coverage new/current, TreeHerder]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f46ec00ec042
[Risks and why]: This is a very simple change, isolated to toolkit/components/extensions/ext-notifications.js so can be considered to be low risk. 
[String/UUID change made/needed]:none

Comment 21

[Bob Silverberg [:bsilverberg]]

As was suggested to me, I exported the commit that I added locally via graft and have attached it to this bug. Hopefully that one will apply cleanly.

Comment 22

[Carsten Book [:Tomcat]]

(In reply to Bob Silverberg [:bsilverberg] from comment #20)
> Created attachment 8760319 [details] [diff] [review]
> Patch to use for uplifting to aurora
> 
> Approval Request Comment
> [Feature/regressing bug #]: This fixes a bug introduced in Bug 1265797, for
> which uplift is also being requested.
> [User impact if declined]: This is a bug in the WebExtensions notifications
> API which is meant to be feature-complete in 48. Users of the API could
> easily hit this bug.
> [Describe test coverage new/current, TreeHerder]:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=f46ec00ec042
> [Risks and why]: This is a very simple change, isolated to
> toolkit/components/extensions/ext-notifications.js so can be considered to
> be low risk. 
> [String/UUID change made/needed]:none

yeah that works too, will wait for tree-reopening. Btw aurora moved now to beta, do you want to land this there too ?

Comment 23

[Bob Silverberg [:bsilverberg]]

(In reply to Carsten Book [:Tomcat] from comment #22)
> 
> yeah that works too, will wait for tree-reopening. Btw aurora moved now to
> beta, do you want to land this there too ?

Yes please, this is meant to be uplifted to 48, which I believe is now beta.

Comment 24

[Carsten Book [:Tomcat]]

(In reply to Bob Silverberg [:bsilverberg] from comment #23)
> (In reply to Carsten Book [:Tomcat] from comment #22)
> > 
> > yeah that works too, will wait for tree-reopening. Btw aurora moved now to
> > beta, do you want to land this there too ?
> 
> Yes please, this is meant to be uplifted to 48, which I believe is now beta.

ok it needs then beta approval from relman

Comment 25

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8760319 [details] [diff] [review]
Patch to use for uplifting to aurora

Approval Request Comment
[Feature/regressing bug #]: This fixes a bug introduced in Bug 1265797, for which uplift is also being requested.
[User impact if declined]: This is a bug in the WebExtensions notifications API which is meant to be feature-complete in 48. Users of the API could easily hit this bug.
[Describe test coverage new/current, TreeHerder]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f46ec00ec042
[Risks and why]: This is a very simple change, isolated to toolkit/components/extensions/ext-notifications.js so can be considered to be low risk. 
[String/UUID change made/needed]:none

Note also, this was already approved for aurora, when 48 was in aurora, but because the merge happened before this was able to land it is now needed in beta too.

Comment 26

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8760319 [details] [diff] [review]
Patch to use for uplifting to aurora

Let's take it to improve WebExtensions, should be in 48 beta 2.

Comment 27

[Carsten Book [:Tomcat]]

has problems uplifting to aurora:

renamed 1275363 -> uplift.patch
applying uplift.patch
patching file toolkit/components/extensions/ext-notifications.js
Hunk #1 FAILED at 48
1 out of 1 hunks FAILED -- saving rejects to file toolkit/components/extensions/ext-notifications.js.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and qrefresh uplift.patch

Comment 28

[Bob Silverberg [:bsilverberg]]

I think the issue is that the patch is already on aurora. We only need to apply it to beta. If I look at the code for aurora [1], I can see the change, whereas the change does not exist on beta [2].

I will remove the request to uplift to aurora.

[1] https://dxr.mozilla.org/mozilla-aurora/source/toolkit/components/extensions/ext-notifications.js#57
[2] https://dxr.mozilla.org/mozilla-beta/source/toolkit/components/extensions/ext-notifications.js#57

Comment 29

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8760319 [details] [diff] [review]
Patch to use for uplifting to aurora

Hmm, I don't seem to be able to remove the flag for aurora uplift approval, but this patch only needs to be uplifted to beta, not aurora.

Comment 30

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/473ca9bf2b2f