Improve behaviour of context menu "block element" button for frames which have lost our click event listeners

[Shikitita]

Environment

Operating System: Windows 8 / Linux Ubuntu
Browser: Chrome 44
ABP version: 1.9.2.1470 / 1.9.3

How to reproduce

1.Install the latest ABP extension for Chrome
2.Access the following website: ​​http://www.skyscanner.co.in/?origin_domain=www.skyscanner.co.in&redirected=true&market=IN
3.Right click on the "Skyscanner" logo at the top left of the page
4.Right click on the "Feedback" tab displayed on the right side of the page
5.Click on "Block element"

Observed behaviour

The "Add filter" dialog displays the filter related to the "Skyscanner" logo instead of the "Feedback" tab. This is noticeable by the logo being highlighted in the background.

Expected behaviour

The filter displayed in the "Add filter" dialog should be related to the "Feedback" tab and not to a previous right-clicked element on the website .
Please note that this issue is related to issue #3137

What to change

For whatever reason the click event listeners for that iframe are lost, so the previous element we right clicked on is still used as the target. We want to offer to block the entire iframe when this happens, instead of the previous element which was right clicked upon.

1. Use the ​frame ID to send the "composer.content.contextMenuClicked" message specifically to the frame for which the content menu click event was fired. (Currently the message is sent to all frames for the page.)
2. Change that message listener so that if lastRightClickEvent is null ​window.frameElement is considered instead. (Will only work if there is a parent frame that is considered of the same origin.)
3. Update the overlay etc code to handle the case of the target element being inside of a different document.

Hints for testers

In both the minimum supported and latest versions of Chrome ensure that:

1. The problem as described above is no longer reproducible.
2. The "block element" tool otherwise works as before.

[kzar]

I can reproduce this as described, investigating.

[kzar]

For some reason it appears that the contextmenu event isn't firing (or the click event) when I'm right clicking on the "Feedback" button in its iframe. This is causing lastRightClickEvent to not be set properly, and therefore the block element tool to not work properly.

I'm now trying to figure out what's going wrong with those events / event listeners.

[kzar]

This bug is also happening when attempting to block the advert at ​http://www.firstcry.com/hair-bands/22/224 at the bottom right of the page. (From #2743)

[kzar]

So I was not able to get to the bottom of this, the content script is being injected into the iframe containing the "Feedback" button, but the contextmenu event listener never seems to fire for it.

Things I noticed, that might be relevant:

• The frame is generated dynamically by some JavaScript
• The frame does not have a src attribute

I've run out of ideas and time for investigating this, unassigning myself for now.

[wspee]

The reason for this seems to be that document.open/write clears the whole document and with it all event listener [0]

I have constructed a test case for this [1]. To see it in action you have to:

• install the extension from the extension directory
• serve the demo.html via any webserver (you can use [2])
• visit demo.html
• open the developer console
• click in the lower iframe a few times -> the console prints "click" for each click
• click on the "load broken" button
• click again in the lower iframe -> the console no longer prints "click"
• after a few seconds the console prints "reconnecting" (now the extension has reconnected the click handler)
• click again in the lower iframe -> the console prints "click" again

The only way I see to fix this would be to patch document.open/write functions analog to wrapWebSocket and I guess we don't want to do that for this?!

So we should close this as "wontfix"?!

[0] ​https://www.w3.org/TR/DOM-Level-2-HTML/html.html#ID-72161170
[1] ​https://github.com/wvspee/wvspee.github.io/tree/master/snippets/3138
[2] ​https://github.com/wvspee/wvspee.github.io/blob/master/simple_http_server.py

[kzar]

Have you confirmed Skyscanner is using document.open/write like you suspect?

The test page you created is a good idea but it would be more useful if it was designed to work with Adblock Plus itself instead of your test extension. Would you mind creating a similar test page which can be used to reproduce the problem described in this issue? (Ideally as one HTML file which also contains the required JavaScript. Then you can link to a hosted copy and developers / testers can use it very simply to reproduce the problem and hopefully in the future confirm that the problem is fixed.)

[wspee]

Replying to kzar:

Have you confirmed Skyscanner is using document.open/write like you suspect?

Not 100% unfortunately. The js is obfuscated and minified which makes it hard to debug it and set brake points. But the src of the iframe is about:blank and there is the following snippet in ​https://www.skyscanner.co.in/sttc/strevda/js/setup.6600bd4dfeb5dd1d4949.js

function(e, t, n) {
    var i = n(13)
      , r = n(59)
      , o = n(36)
      , s = n(33)("IE_PROTO")
      , a = function() {}
      , f = "prototype"
      , l = function() {
        var e, t = n(18)("iframe"), i = o.length, r = "<", s = ">";
        for (t.style.display = "none",
        n(60).appendChild(t),
        t.src = "javascript:",
        e = t.contentWindow.document,
        e.open(),
        e.write(r + "script" + s + "document.F=Object" + r + "/script" + s),
        e.close(),
        l = e.F; i--; )
            delete l[f][o[i]];
        return l()

Do you know how I could 100% verify that skyscanner is indeed using open/write to create this iframe?

The test page you created is a good idea but it would be more useful if it was designed to work with Adblock Plus itself instead of your test extension. Would you mind creating a similar test page which can be used to reproduce the problem described in this issue? (Ideally as one HTML file which also contains the required JavaScript. Then you can link to a hosted copy and developers / testers can use it very simply to reproduce the problem and hopefully in the future confirm that the problem is fixed.)

I have update the demo.html to include images you can block with the normal abp extension. Simply right click the image before and after loading the broken version and the "Block element" contextmenu entry stops working.

[kzar]

Thanks for investigating, I have updated the issue. I don't think there is an obvious way to proceed here and the amount of work required to continue with this issue is probably not worth it for now, we have more important stuff to worry about.

One thing you could do would be to host the test page somewhere and link to that from the issue description as an alternative way to reproduce the problem. Other than that I would probably recommend focusing on other issues for now, I think our time is better spent elsewhere.

I've assigned myself and will upload a small fix to at least avoid the wrong element from being highlighted for now. After that I'll leave this issue for a rainy day.

[kzar]

Whoops, since the user did not necessarily click our context menu button after the context menu was opened we can't rely on that listener to clear the last right clicked element state. I've updated the issue description again. Unless we can figure out how to re-add our click event listeners when they're lost I don't think this issue is worth pursuing for now. The workaround I've suggested in the description for this edge case will introduce quite a bit of complexity and IMO is probably not worth it.

[kzar]

Scratch that I've figured out a way this can work.

[kzar]

I just rebased the changes and noticed a mistake. How I was checking for Edge was wrong, I think the right way would be to use the info module to check the platform. But for that we need to be able to require modules from the content scripts, so this is now blocked by #5080.

[sebastian]

Having the info module included in the bundle for the content scripts seems to be a bug to me. If you need information from the info module in the content script you can query them from the background page, like we do ​everywhere else.

[kzar]

Well I disagree that requiring the info module from the content scripts is a bug, and we do it with messaging everywhere else because before #5080 landed that was the only way to do it. (Perhaps we could change that throughout the content scripts and save some messaging?)

But fair enough, I don't mind doing it with messaging for now. I'll update the review.

[sebastian]

My point rather is that we should not include the info module in the content scripts in the first place, to avoid having redundant code in the builds, and executing it redundantly at run time.

[kzar]

I can't reproduce this bug any more, I tested with Chrome 69 and devenv extension built from ​0404a2247ebf. If anyone else can, or knows another website where this problem occurs let me know. In the mean time I'm closing this.