Opened 6 days ago

Closed 2 days ago

Last modified 4 hours ago

#7206 closed defect (fixed)

Notifications crash Edge

Reported by: Ross Assignee: kzar
Priority: P2 Milestone: Adblock-Plus-for-Chrome-Opera-Firefox-next
Module: Platform Keywords: externaldependency
Cc: sebastian, kzar, geo Blocked By:
Blocking: Platform: Edge
Ready: yes Confidential: no
Tester: Ross Verified working: yes
Review URL(s):

https://gitlab.com/eyeo/adblockplus/adblockpluschrome/merge_requests/25

Description (last modified by kzar)

Environment

ABP 0.9.11.2214 (Devbuild), or 0.9.11 (Release)
Edge 44.17763.1.0 / EdgeHTML 18.17763

How to reproduce

Intercept and rewrite the request to notification.adblockplus.org using a proxy to return a notification file.

OR

Append this snippet to adblockpluschrome/lib/csp.js, rebuild and reload the extension, then navigate to https://example.com:

require("notification").Notification.addNotification({
  id: new Date() | 0,
  type: "critical",
  title: "Notification title",
  message: "<a>Open contribution page</a>",
  links: ["contribute"],
  urlFilters: ["||example.com^$document"]
});

Observed behaviour

Edge crashes just after ABP makes the request for the notification file. The same notification file works fine in Chrome.

Expected behaviour

Notifications to not crash the browser.

Notes

Even simply calling the notification API from the extension's background console crashes Edge consistently! For example, try this snippet:

browser.notifications.create({
  type: "basic",
  iconUrl: "icons/abp-16.png",
  title: "crash test",
  message: "hello world"
});

See the related Microsoft Edge bug report: https://developer.microsoft.com/en-us/microsoft-edge/platform/issues/20146233/

Hints for testers

As a workaround we've stopped using the browser.notifications API on Edge. Ensure that notifications appear in the popup window instead on Edge, and that notifications still show up as normal on other platforms.

Attachments (1)

Change History (17)

comment:1 Changed 6 days ago by kzar

  • Component changed from Unknown to Platform
  • Description modified (diff)
  • Priority changed from Unknown to P2
  • Ready set

comment:2 Changed 6 days ago by kzar

  • Description modified (diff)

comment:3 Changed 6 days ago by kzar

  • Description modified (diff)
  • Keywords externaldependency added

Edge crashes when we use the browser.notifications.create API, it happens with the currently released version of Adblock Plus too. I've reported this to Microsoft, but I don't think this should block our release.

comment:4 Changed 6 days ago by kzar

Since apparently the Edge issue tracker is also having troubles, I'll attach a copy of my report here too...

comment:5 follow-up: Changed 6 days ago by sebastian

What happens when navigating to a website targeted by the Adblock Warning Removal List? If this is causing Microsoft Edge to crash, we might want to address this issue before the release. Though not a regression, this would be quite critical. I suppose, if nothing else, we could just make sure to not use the notifications API on Microsoft Edge.

Last edited 6 days ago by sebastian (previous) (diff)

comment:6 in reply to: ↑ 5 Changed 6 days ago by Ross

Can confirm it also crashes when just visiting a site targeted by the Adblock Warning Removal List (and triggering the notification).

comment:7 Changed 5 days ago by kzar

  • Review URL(s) modified (diff)
  • Status changed from new to reviewing

I suppose, if nothing else, we could just make sure to not use the notifications API on Microsoft Edge.

I guess you're right, we should probably just avoid using browser.notifications entirely on Edge until Microsoft sort this out.

comment:8 Changed 5 days ago by kzar

  • Owner set to kzar

comment:9 Changed 2 days ago by abpbot

A commit referencing this issue has landed:
Issue 7206 - Avoid using browser.notifications API on Edge!

comment:10 Changed 2 days ago by kzar

  • Milestone set to Adblock-Plus-for-Chrome-Opera-Firefox-next
  • Resolution set to fixed
  • Status changed from reviewing to closed

(I don't see a Adblock-Plus-For-Edge-next milestone, so hopefully this milestone is OK.)

comment:11 Changed 2 days ago by kzar

  • Description modified (diff)

comment:12 Changed 2 days ago by kzar

  • Milestone Adblock-Plus-for-Chrome-Opera-Firefox-next deleted

comment:13 Changed 2 days ago by kzar

  • Milestone set to Adblock-Plus-for-Chrome-Opera-Firefox-next

comment:14 Changed 27 hours ago by Ross

This isn't fixed in the build below yet. Although Sebastian mentioned there should be a 2221 build soon.

ABP 0.9.11.2216 (Devbuild)
Edge 44.17763.1.0 / EdgeHTML 18.17763

comment:15 Changed 22 hours ago by sebastian

Yes, this was fixed in r2218, but the current devbuild is still at 2216. Build #2223 is currently waiting to get published by Microsoft, but as long as we land new changes in master at a higher rate than the Windows Store processes submissions, we don't get a new devbuild out. ;)

comment:16 Changed 4 hours ago by Ross

  • Tester changed from Unknown to Ross
  • Verified working set

Ah okay, that's useful to know.

This is fixed. Doesn't crash when receiving notifications from the notificationurl or when visiting a site on the adblock warning removal list.

ABP 0.9.11.2223 (Devbuild)
Edge 44.17763.1.0 / EdgeHTML 18.17763

Note: See TracTickets for help on using tickets.