Issue reporter is broken on Edge

[Ross]

Environment

ABP 0.9.11.2206 (Devbuild)
Edge 42.17134.1.0 / EdgeHTML 17.17134

Regression compared to 0.9.11 (Release)

How to reproduce

1. Navigate to a website.
2. Select [ABP Icon] > [Report issue].

Observed behaviour

The issue reporter opens, but a pop up is displayed with an error message and then the issue reporter is automatically closed.

Error: Incorrect function

Expected behaviour

Issue reporter to be usable.

[geo]

I've taken a look, so far the problem seems to be in issue-reporter-report.js -> collectRequests -> browser.tabs.create({active: false, url: tab.url}), that code fails for some reason.

Also, I'm seeing this warning in the console CSP14312: Resource violated directive 'script-src 'self'' in Host Defined Policy: inline script. Resource will be blocked. but I'm not sure what might be causing it.

[saroyanm]

Replying to geo:

I've taken a look, so far the problem seems to be in issue-reporter-report.js -> collectRequests -> browser.tabs.create({active: false, url: tab.url}), that code fails for some reason.

Thanks for the hints.

That's indeed weird, though the error seem to be only thrown when the Promise.all is used, so basically snippet below output error:

function getPromise()
{
  return browser.tabs.create({'active': false, 'url': 'http://google.com'});
}
Promise.all([getPromise()]).then(() =>
{
  console.log('ready');
}).catch(()=>
{
  console.log('error');
});

Update: This only happens in the ABP, so I would assume that might somehow be connected with the Polyfill we use .
Update2: I can now confirm that the issue is somehow related to the Polyfill and seems like browser.tabs.create is not only one affected by that.

[saroyanm]

Using Callback parameter of the browser.tabs.create seems to fix the issue, though I'm not sure if this is a good solution though.

Even if we decide to use that solution temporary we might want to ensure that change doesn't affect other platforms as well.

[saroyanm]

Added a ​workaround to fix the error caused by browser.tabs.create, but still the issue reporter is broken:

• No screenshot is loadded
• The report not being sent

Also noticed that updating dependencies to latest adblockplusui master causes the issue reporter ​not to open at all.

[sebastian]

Replying to saroyanm:

Using Callback parameter of the browser.tabs.create seems to fix the issue, though I'm not sure if this is a good solution though.

Yes, we should definitely fix the polyfill, rather than going back to passing callbacks around.

Replying to saroyanm:

Also noticed that updating dependencies to latest adblockplusui master causes the issue reporter ​not to open at all.

That is to be expected, and most certainly not the only thing that is broken. The last 3 commits (as of f86abf2efdfd) in adblockplusui adapt for changes in adblockpluscore. We plan to update the adblockpluscore and adblockplusui dependencies simultaneously in next@adblockpluschrome (#7054). Unless this bug can be fixed within adblockpluschrome, you have to back out those changes for now (or create a branch) before landing a fix in adblockplusui.

[agiammarchi]

This issue is super weird.

Just to clarify a couple of things:

1. the polyfill works
2. the screenshot works too
3. previous points work only through the popup though

If I run the screenshot callback from the issue reporter, opened through the popup via:

function reportIssue(tab)
{
  browser.tabs.create({
    active: false,
    url: browser.runtime.getURL("/issue-reporter.html?" + tab.id)
  }).then(
    // force closing popup which is not happening in Firefox
    // @link https://issues.adblockplus.org/ticket/7017
    () => window.close()
  );
}

which demonstrates that indeed the polyfill works as expected, I always receive undefined, as screenshot result.

browser.tabs.captureVisibleTab(
  null,
  {},
  screenshot =>
  {
    // always undefinedin EdgeHTML 18
    window.console.info(screenshot);
  }
);

However, if I invoke the exact same code from the popup, I always receive the expected data source URI.

After quite few experiments through IDs, different image format, Promise/not-promise, I think I start believing there is some CSP issue that is messing up with non popup pages.

Unfortunately it is very hard to find the culprit of the error and the behavior.

[agiammarchi]

FYI, I've done dozen different tests and concluded this is not a UI bug but a Web Ext API one, related to Edge only.

All UI does is using such API and nothing literally work as expected, which is weird, but from the popup everything works fine.

I haven't managed to find the source of the CSP issue thought, but if I can help somehow let me know where to look in either core or chrome repo, thanks.

[sebastian]

If it's true that this is a bug in Microsoft Edge with no available workaround, we should do two things:

1. File an issue in the ​Microsoft Edge issue tracker.
2. Update adblockplusui to not provide the issue reporter on Microsoft Edge, or maybe just disable the screenshot feature on Microsoft Edge.

[agiammarchi]

I'm theoretically on vacation today but since I've read this anyway, I'd like to quickly answer about your points:

1. I wouldn't know what to file exactly, beside telling them the screenshot doesn't work from new tabs. Do you think that would be enough? I was rather hoping someone with more core/API experience could double check my conclusions first.
2. the Issue Reporter flow is designed around the screenshot, there are steps, and text, related to such feature. Re-adapting the whole thing for Edge only might take much more time than simply hiding the report from Edge, but I guess this is not my call.

Regardless what we decide to do, this bug is not strictly UI related so maybe we need to modify this ticket to core?

[sebastian]

Replying to agiammarchi:

1. I wouldn't know what to file exactly, beside telling them the screenshot doesn't work from new tabs. Do you think that would be enough? I was rather hoping someone with more core/API experience could double check my conclusions first.

That browser.tabs.captureVisibleTab() from an extension page in a tab always responds with undefined is exactly what we should report.

2. the Issue Reporter flow is designed around the screenshot, there are steps, and text, related to such feature. Re-adapting the whole thing for Edge only might take much more time than simply hiding the report from Edge, but I guess this is not my call.

I happily leave it up to you guys, whether to hide the whole issue reporter, or just not create a screenshot, on Microsoft Edge. But we should do either soon, so that we can move forward with the release of Adblock Plus 0.9.12 for Microsoft Edge.

Regardless what we decide to do, this bug is not strictly UI related so maybe we need to modify this ticket to core?

How so? At the current point it seems the only thing we can to do here is disabling some UI functionality on Microsoft Edge.

[erezson]

Based on the information I got from Winsley, the issue reporter is not useful without the screenshot. So please disable it completely for Edge

[agiammarchi]

But we should do either soon

It's done in latest review URL, MS Edge won't show at all the Issue Reporter button.

However, I'm not 100% sure how / where to eventually push that patch once approved, but I guess we'll discuss this when that's approved.

[agiammarchi]

P.S. I've also filed a bug in MS bug tracker, after checking it wasn't a known issue: ​https://developer.microsoft.com/en-us/microsoft-edge/platform/issues/20064213/

[sebastian]

Thanks! Once this change is approved, can you update adblockplusui@hg, backing out acc8079d906d, f86abf2efdfd and cc4d5ca74953, and landing this fix there, so that when I update the adpluckplusui dependency in master@adblockpluschrome the only change pulled in is this fix?

[agiammarchi]

Replying to sebastian:

can you update adblockplusui@hg, backing out acc8079d906d, f86abf2efdfd and 1eae7550e9e1, and landing this fix there ... ?

I cannot find 1eae7550e9e1 after an update + purge, but I'm still not super familiar with hg and I don't have Thomas knowledge about other two logs (is it safe to remove those?)

On top of that, I'm not sure what's the best way to back out those and put those back due automations we have on GitLab. What if I branch hg at certain point and apply a patch there?

I also don't think I have access to hg to do anything ... in few words, any help would be appreciated!

[saroyanm]

Replying to agiammarchi:

Replying to sebastian:

can you update adblockplusui@hg, backing out acc8079d906d, f86abf2efdfd and 1eae7550e9e1, and landing this fix there ... ?

I cannot find 1eae7550e9e1 after an update + purge, but I'm still not super familiar with hg and I don't have Thomas knowledge about other two logs (is it safe to remove those?)

I assume what Sebastian meant instead of 1eae7550e9e1(This seem to refer to ABPCHROME) to backout cc4d5ca74953, which is part of #7027, as we haven't updated the Core dependency, those changes are causing the Build to be broken. Also for the reference -> https://issues.adblockplus.org/ticket/7175#comment:9

Sebastian can you please confirm that it's accurate, before we start backing out and applying Andreas fix on top ?

[sebastian]

Well spotted. Indeed that third commit hash was wrong. I updated my comment above.

Also yes, instead of backing out those 3 commits, we could branch from 2c0512a95dbb. But it seems in the past you guys went for backing out commits from master@hg instead.

[abpbot]

A commit referencing this issue has landed:
​Issue 7175 - Hide Issue Reporter in MS Edge

[abpbot]

A commit referencing this issue has landed:
​Issue 7175 - Updated adblockplusui dependency in order to hide incompatible issue reporter on MS Edge

[Ross]

Fixed via Issue Reporter always being hidden on Edge.

ABP 0.9.11.2214 (Devbuild)
Edge 44.17763.1.0 / EdgeHTML 18.17763

[abpbot]

A commit referencing this issue has landed:
​Issue 7175 - Hide Issue Reporter in MS Edge