Opened 9 months ago

Closed 9 months ago

Last modified 4 months ago

#7175 closed defect (fixed)

Issue reporter is broken on Edge

Reported by: Ross Assignee: agiammarchi
Priority: P2 Milestone: Adblock-Plus-3.4.3-for-Chrome-Opera-Firefox
Module: User-Interface Keywords:
Cc: sebastian, kzar, greiner, geo, agiammarchi, saroyanm, mjethani Blocked By:
Blocking: #6936 Platform: Edge
Ready: yes Confidential: no
Tester: Ross Verified working: yes
Review URL(s):

https://gitlab.com/eyeo/adblockplus/abpui/adblockplusui/merge_requests/185

Description

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.

Change History (31)

comment:1 Changed 9 months ago by Ross

  • Cc sebastian added; snoack removed

comment:2 follow-up: Changed 9 months ago by 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.

comment:3 Changed 9 months ago by sebastian

  • Cc agiammarchi added

comment:4 Changed 9 months ago by saroyanm

  • Cc saroyanm added

comment:5 in reply to: ↑ 2 Changed 9 months ago by 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.

Last edited 9 months ago by saroyanm (previous) (diff)

comment:6 Changed 9 months ago by saroyanm

  • Cc mjethani added

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.

comment:7 Changed 9 months ago by saroyanm

  • Priority changed from Unknown to P2
  • Ready set

comment:8 follow-up: Changed 9 months ago by saroyanm

  • Review URL(s) modified (diff)

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.

Last edited 9 months ago by saroyanm (previous) (diff)

comment:9 in reply to: ↑ 8 Changed 9 months ago by 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.

comment:10 Changed 9 months ago by 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.

comment:11 Changed 9 months ago by 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.

comment:12 Changed 9 months ago by 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.

comment:13 follow-up: Changed 9 months ago by 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?

comment:14 in reply to: ↑ 13 Changed 9 months ago by 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.

  1. 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.

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

comment:15 Changed 9 months ago by 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

comment:16 Changed 9 months ago by agiammarchi

  • Review URL(s) modified (diff)

comment:17 Changed 9 months ago by 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.

comment:18 Changed 9 months ago by 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/

comment:19 follow-up: Changed 9 months ago by 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?

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

comment:20 in reply to: ↑ 19 ; follow-up: Changed 9 months ago by 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!

comment:21 in reply to: ↑ 20 Changed 9 months ago by 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 ?

comment:22 Changed 9 months ago by 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.

comment:23 Changed 9 months ago by abpbot

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

comment:25 Changed 9 months ago by sebastian

  • Owner set to agiammarchi

comment:26 Changed 9 months ago by sebastian

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

comment:27 Changed 8 months ago by Ross

  • Tester changed from Unknown to Ross
  • Verified working set

Fixed via Issue Reporter always being hidden on Edge.

ABP 0.9.11.2214 (Devbuild)
Edge 44.17763.1.0 / EdgeHTML 18.17763

comment:28 Changed 8 months ago by kzar

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

comment:29 Changed 8 months ago by sebastian

  • Milestone changed from Adblock-Plus-for-Chrome-Opera-Firefox-next to Adblock-Plus-3.4.3-for-Chrome-Opera-Firefox

comment:30 Changed 4 months ago by abpbot

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

comment:31 Changed 4 months ago by kzar

  • Blocking 6936 added
Note: See TracTickets for help on using tickets.