Closed Bug 1275363 Opened 3 years ago Closed 3 years ago

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

Categories

(WebExtensions :: Untriaged, defect, P2)

defect

Tracking

(firefox47 wontfix, firefox48 fixed, firefox49 fixed)

RESOLVED FIXED
mozilla49
Iteration:
49.3 - Jun 6
Tracking Status
firefox47 --- wontfix
firefox48 --- fixed
firefox49 --- fixed

People

(Reporter: bsilverberg, Assigned: bsilverberg)

Details

Attachments

(1 file, 1 obsolete file)

Assignee: nobody → bob.silverberg
Status: NEW → ASSIGNED
Iteration: --- → 49.3 - Jun 6
Priority: -- → P2
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 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/
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.
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.
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 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
Attachment #8756012 - Flags: review?(kmaglione+bmo) → review+
Keywords: checkin-needed
(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
https://hg.mozilla.org/mozilla-central/rev/f59918cf04fa
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
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.
Attachment #8756012 - Flags: approval-mozilla-aurora?
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
Attachment #8756012 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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')
Flags: needinfo?(bob.silverberg)
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.
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?
Flags: needinfo?(bob.silverberg) → needinfo?(cbook)
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
Attachment #8756012 - Attachment is obsolete: true
Attachment #8760319 - Flags: approval-mozilla-aurora?
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.
(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 ?
Flags: needinfo?(cbook) → needinfo?(bob.silverberg)
(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.
Flags: needinfo?(bob.silverberg) → needinfo?(cbook)
(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
Flags: needinfo?(cbook)
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.
Attachment #8760319 - Flags: approval-mozilla-beta?
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.
Attachment #8760319 - Flags: approval-mozilla-beta?
Attachment #8760319 - Flags: approval-mozilla-beta+
Attachment #8760319 - Flags: approval-mozilla-aurora?
Attachment #8760319 - Flags: approval-mozilla-aurora+
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
Flags: needinfo?(bob.silverberg)
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
Flags: needinfo?(bob.silverberg)
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.
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.