#6821 closed defect (duplicate)

Extension broken on Chrome 49

Reported by: Ross Assignee:
Priority: P1 Milestone:
Module: Platform Keywords:
Cc: sebastian, kzar, greiner, hfiguiere, mjethani Blocked By: #6823
Blocking: Platform: Chrome
Ready: yes Confidential: no
Tester: Unknown Verified working: no
Review URL(s):

Description (last modified by kzar)

Environment

ABP 3.2.0.2097
Chrome 49

Works fine in Chrome 55+.
Smoke tested with Chrome 50.
Not able to check in Chrome 51-54.

Is fine in Firefox's min supported version (51)

How to reproduce

  1. Install extension.
  2. Attempt to open options page.

Observed behaviour

The first run page does not open. The options page cannot be opened from the popup UI. Ad blocking does not work.

Some errors from the background console:

    Uncaught SyntaxError: Invalid regular expression flags    adblockplus.js:6274

some errors from the popup console:

    extensions::uncaught_exception_handler:8 Error in event handler for (unknown): TypeError: Cannot read property 'locale' of undefined
    at browser.runtime.sendMessage (chrome-extension://ldcecbkkoecffmfljeihcmifjjdoepkn/i18n.js:31:47)handler @ extensions::uncaught_exception_handler:8

    extensions::uncaught_exception_handler:8 Error in event handler for (unknown): TypeError: Cannot read property 'toLocaleString' of undefined
    at browser.runtime.sendMessage.blockedPage (chrome-extension://ldcecbkkoecffmfljeihcmifjjdoepkn/popup.js:177:41)handler @ extensions::uncaught_exception_handler:8

    extensions::uncaught_exception_handler:8 Error in event handler for (unknown): TypeError: Cannot read property 'toLocaleString' of undefined
    at blockedTotal (chrome-extension://ldcecbkkoecffmfljeihcmifjjdoepkn/popup.js:184:42)

Expected behaviour

The extension to work as expected in Chrome 49.

Notes

Change History (25)

comment:1 Changed 15 months ago by kzar

Uncaught SyntaxError: Invalid regular expression flags adblockplus.js:6274

It seems that Chrome 49 doesn't support the "u" flag for regular expressions, this exception in particular is caused by 5f851931ffea in adblockpluscore/lib/snippets.js, but I can see 926b9df46ce6 added one to adblockpluscore/lib/common.js, and 7052e279497a added one to adblockpluscore/test/browser/elemHideEmulation.js.

comment:3 Changed 15 months ago by kzar

I can't see any obvious cause of those popup console exceptions, I suspect they might just be a side-effect of the exceptions thrown in the background console. Especially the ones that relate to the responses to messages sent to the background being undefined.

comment:4 Changed 15 months ago by kzar

  • Description modified (diff)
  • Priority changed from Unknown to P1
  • Ready set

comment:5 Changed 15 months ago by kzar

Ross please can you confirm if Adblock Plus 3.2 works correctly with Chrome 49?

comment:6 Changed 15 months ago by hfiguiere

I think this raises the question whether we should support Chrome 49? It seems that it is because it is the last version supported by Windows XP, but then both are out of security updates. And for the other users, Google update push make it unlikely for users to not run the latest....

comment:7 Changed 15 months ago by Ross

3.1.0.2069 (the last 3.1 devbuild) and 3.2.0.2087 (the first 3.2 devbuild) seem to work correctly in Chrome 49.

comment:8 Changed 15 months ago by kzar

Dang alright, well I guess this is a release blocker unless we decide to drop Chrome 49 support then. Any opinion Sebastian?

comment:9 Changed 15 months ago by Ross

Just to note, the testing virtual machines we use only have Chrome 49, 55, 60-68+ on them at the moment. Although if needed I could probably find a sketchy installer for 50-54 somewhere if we say bumped to one of those for min version.

comment:10 Changed 15 months ago by kzar

Yea, it would be kind of useful to confirm that the extension works properly with Chrome 50 before we make a decision about dropping Chrome 49.

comment:11 follow-up: Changed 15 months ago by Ross

The 3.2.0.2097 devbuild appears to work in Chrome 50 after a quick check. The above console errors are not thrown and first run page, functionality appear to be working okay too.

Last edited 15 months ago by Ross (previous) (diff)

comment:12 Changed 15 months ago by greiner

  • Cc greiner added

comment:13 in reply to: ↑ 11 Changed 15 months ago by kzar

  • Description modified (diff)

Replying to Ross:

The 3.2.0.2097 devbuild appears to work in Chrome 50 after a quick check. The above console errors are not thrown and first run page, functionality appear to be working okay too.

Thanks for confirming that, I've updated the description.

comment:14 follow-up: Changed 15 months ago by sebastian

If there is any benefit in dropping support for Chrome 49, we can request the usage data, and look into it. But for this release, we might be moving forward faster if we just fix the incompatibility.

Also if it's just for using the u flag in regular expressions, the examples where we apparently just started to use it don't seem to make a strong case. As far as I can tell, how the u flag is used in snippet.js and common.js, it is redudnant, i.e. those regular expressions are equivalanet to the same regular expression without the u flag since they neither use unicode-escapes nor the . character. In test/browser/elemHideEmulation.js we specifically test support of regular expressions with u flag with -abp-contains(). We can either just remove this test, or skip it if the u flag isn't supported.

Last edited 15 months ago by sebastian (previous) (diff)

comment:15 Changed 15 months ago by sebastian

  • Cc sebastian added; snoack removed

comment:16 in reply to: ↑ 14 Changed 15 months ago by kzar

  • Cc hfiguiere mjethani added

Replying to sebastian:

If there is any benefit in dropping support for Chrome 49, we can request the usage data, and look into it. But for this release, we might be moving forward faster if we just fix the incompatibility.

Also if it's just for using the u flag in regular expressions, the examples where we apparently just started to use it don't seem to make a strong case. As far as I can tell, how the u flag is used in snippet.js and common.js, it is redudnant, i.e. those regular expressions are equivalanet to the same regular expression without the u flag since they neither use unicode-escapes nor the . character. In test/browser/elemHideEmulation.js we specifically test support of regular expressions with u flag with -abp-contains(). We can either just remove this test, or skip it if the u flag isn't supported.

Manish/Hubert Please could you open a core issue to remove use of the "u" flag, or to skip its use when it isn't supported therefore? Please mark it as blocking this issue, then this issue can turn into a dependency update.

comment:17 Changed 15 months ago by mjethani

My bad, indeed the u flag is useless in those cases.

Now do I understand correctly that doing another dependency update will cause the release to be delayed? I doubt that Chrome 49 has any more than 0.1% of the overall market share. It might be better instead to drop support for Chrome 49.

In any case let me open an issue for this and submit a patch.

Last edited 15 months ago by mjethani (previous) (diff)

comment:18 Changed 15 months ago by mjethani

  • Blocked By 6823 added

comment:19 Changed 15 months ago by sebastian

Last time (not too long ago), we looked into it, Chrome 49 had significantly more users than that. Anyway, getting updated usage data and then having a debate over it will likely take longer than just fixing this regression.

comment:20 Changed 15 months ago by hfiguiere

  • Blocked By 6824 added

comment:21 Changed 15 months ago by hfiguiere

  • Blocked By 6824 removed

comment:22 Changed 15 months ago by mjethani

#6823 is closed.

comment:23 Changed 15 months ago by mjethani

  • Blocked By 6825 added

comment:24 Changed 15 months ago by sebastian

  • Blocked By 6825 removed

comment:25 Changed 15 months ago by sebastian

  • Resolution set to duplicate
  • Status changed from new to closed

Closing this issue as duplicate since there is a new issue for the dependency update that will fix this.

Note: See TracTickets for help on using tickets.