Opened 4 years ago

Closed 8 months ago

#3138 closed defect (worksforme)

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

Reported by: Shikitita Assignee: kzar
Priority: P4 Milestone:
Module: Platform Keywords:
Cc: sebastian, mapx, kzar, wspee Blocked By:
Blocking: Platform: Chrome
Ready: yes Confidential: no
Tester: Unknown Verified working: no
Review URL(s):

https://codereview.adblockplus.org/29370947/

Description (last modified by kzar)

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.

Change History (20)

comment:1 Changed 4 years ago by mapx

  • Cc sebastian mapx added

comment:2 Changed 4 years ago by kzar

  • Component changed from Unknown to Platform
  • Owner set to kzar
  • Priority changed from Unknown to P2
  • Ready set

I can reproduce this as described, investigating.

comment:3 Changed 4 years ago by 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.

Last edited 4 years ago by kzar (previous) (diff)

comment:4 Changed 4 years ago by 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)

comment:5 Changed 4 years ago by kzar

  • Priority changed from P2 to P3

comment:6 Changed 4 years ago by kzar

  • Owner kzar deleted

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.

comment:7 Changed 2 years ago by wspee

  • Cc kzar added
  • Owner set to 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

comment:8 follow-up: Changed 2 years ago by 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.)

Last edited 2 years ago by kzar (previous) (diff)

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

comment:10 Changed 2 years ago by kzar

  • Cc wspee added
  • Description modified (diff)
  • Owner changed from wspee to kzar
  • Priority changed from P3 to P4
  • Ready unset
  • Summary changed from [ABP for Chrome] "Add filter" dialog displays information from previous right-click to Improve behaviour of "block element" from context menu within frames when our click event listeners are lost

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.

comment:11 Changed 2 years ago by kzar

  • Description modified (diff)

comment:12 Changed 2 years ago by kzar

  • Description modified (diff)
  • Owner kzar deleted

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.

comment:13 Changed 2 years ago by kzar

  • Description modified (diff)
  • Owner set to kzar
  • Ready set
  • Summary changed from Improve behaviour of "block element" from context menu within frames when our click event listeners are lost to Improve behaviour of context menu "block element" button for frames which have lost our click event listeners

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

comment:14 Changed 2 years ago by kzar

  • Description modified (diff)
  • Milestone set to Adblock-Plus-for-Chrome-Opera-next
  • Review URL(s) modified (diff)
  • Status changed from new to reviewing

comment:15 Changed 2 years ago by sebastian

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

comment:16 Changed 22 months ago by kzar

  • Blocked By 5080 added

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.

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

comment:18 Changed 19 months ago by kzar

  • Blocked By 5080 removed

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.

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

comment:20 Changed 8 months ago by kzar

  • Resolution set to worksforme
  • Status changed from reviewing to closed

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.

Note: See TracTickets for help on using tickets.