Opened 5 weeks ago

Last modified 5 days ago

#7104 new defect

Element collapsing stylesheets can linger after frame is destroyed

Reported by: Ross Assignee: mjethani
Priority: P2 Milestone:
Module: Platform Keywords:
Cc: sebastian, kzar, mjethani Blocked By:
Blocking: Platform: Firefox
Ready: yes Confidential: no
Tester: Unknown Verified working: no
Review URL(s):

https://codereview.adblockplus.org/29958567/

Description (last modified by Ross)

Environment

ABP 3.4.0.2187
Firefox 62

Could not reproduce in Chrome 70.

How to reproduce

  1. Create a basic HTML page with two images.
  2. Add a filter that blocks both images.
  3. Refresh page, filters apply.
  4. Change filter added in Step 2 to just block one image.
  5. Refresh the page.

Observed behaviour

After Step 4 and 5, both images are still hidden in Firefox (but not in Chrome).

Closing the tab, waiting a few seconds and reopening the page seems to fix it but not if done too quickly.

Expected behaviour

Firefox to behave as Chrome does.

Example

HTML:

<!DOCTYPE html>
<head>
<title>Block test</title>
</head>
<body>
  <h1>Block test</h1>
  <img src="test1.jpg"/>
  <img src="test2.jpg"/>
</body>
</html>

Filters:

Step 2:

test*.jpg

Step 4:

test1.jpg

Attachments (1)

7104-rules..jpg (227.9 KB) - added by Ross 3 weeks ago.

Download all attachments as: .zip

Change History (15)

comment:1 Changed 5 weeks ago by Ross

I noticed this while writing a new genericblock test page.

The page has two images that were blocked with these two filters:
/testcasefiles/genericblock/image.jpg
/testcasefiles/genericblock/image2.jpg$domain=localhost

Which ABP reports as 2 blocked items in the popup icon and both images are correctly blocked.

Adding the following filter and refreshing:
@@localhost^$genericblock

Causes ABP to report 1 on the popup icon and show the genericblock filter in the devtools panel. However both images are still blocked (or set to display: none).

Then, closing the tab and reopening the page in a new tab causes the second image to appear as expected (however this does not always work)

Last edited 5 weeks ago by Ross (previous) (diff)

comment:2 Changed 5 weeks ago by mjethani

  • Cc mjethani added; manish removed

comment:3 Changed 3 weeks ago by kzar

Please could you take a look into this one Manish?

comment:4 Changed 3 weeks ago by mjethani

  • Owner set to mjethani

comment:5 Changed 3 weeks ago by mjethani

Ross, what is the element hiding filter here? What I see in the description above is a blocking filter.

comment:6 in reply to: ↑ description Changed 3 weeks ago by mjethani

Replying to Ross:

The CSS rule to hide the image looks to be injected multiple times on page refresh.

How exactly do you see this CSS rule, and what does it look like?

comment:7 Changed 3 weeks ago by Ross

  • Description modified (diff)

Changed 3 weeks ago by Ross

comment:8 follow-up: Changed 3 weeks ago by Ross

You're right, it's a blocking filter. I feel like I gave a bad explaination, I've updated the example above. I think there might actually be two issues here (but I also might be wrong):

  1. Firefox still hides/blocks an image that was previously the target of the filter, but is no longer (the filter changed). So in the updated example above, test2.jpg is still being hidden/blocked even though the filter is now test1.jpg instead of test*.jpg. The ABP icon also indicates just 1 blocked item. This is fixed after closing/reopening the tab. Also "fixed" when removing the filters.
  1. The second issue is that, when inspecting the image element pointing at test1.jpg in the example above, the number of display:none rules applied to it is dependent on the number of times the page has been refreshed. Each refresh adds another. This isn't exactly incorrect, as the image is hidden correctly. However the extra rules on page refresh seems to indicate something weird is happening somewhere? Chrome does not have the same behaviour.
Last edited 3 weeks ago by Ross (previous) (diff)

comment:9 Changed 8 days ago by mjethani

I have not looked into this yet, but I just wanted to mention that this might be related to our work on element collapsing (#5899, #6645).

I'll take a closer look after the next dependency update (#7054).

comment:10 in reply to: ↑ 8 Changed 8 days ago by mjethani

Replying to Ross:

I think there might actually be two issues here [...]

This looks serious.

Based on your description, they both look like symptoms of the same problem to me: the previous style sheet is somehow still there and being applied.

comment:11 Changed 8 days ago by mjethani

First of all, I'm seeing the issue on both Chrome and Firefox.

Technical details: When a style sheet is injected to "collapse" a blocked media element (image, video, etc.), it only appends to the existing style sheet. But the style sheet is never cleared, so it keeps appending.

This is bad because if you navigate to a different page with similar elements (like elements matching the CSS selector img[src="test1.jpg"]), the elements on the new page would also get hidden, even though the media would still be played!

Ross, you could construct a worse case here where there are two pages, and a video is blocked only on the first page. For example, let's say there's a filter /foo.mp4^$domain=foo.com, which would block the URL /foo.mp4 on foo.com but not bar.com. If you go to the page on foo.com, the URL would be blocked and the video element would be collapsed. If you then navigate in the same tab to bar.com, the video would not be blocked, but the element would still be collapsed (because of the stale style sheet), so you would hear the sound but not see anything.

comment:12 Changed 8 days ago by mjethani

A tentative fix is to add the following line at the end of the updateFrameStructure function in ext/background.js:

frame.injectedStyleSheets = null;

comment:13 Changed 8 days ago by mjethani

  • Review URL(s) modified (diff)

Added a patch for a potential fix.

comment:14 Changed 5 days ago by kzar

  • Priority changed from Unknown to P2
  • Ready set
  • Summary changed from Duplicate rules are applied to element on page refresh on Firefox to Element collapsing stylesheets can linger after frame is destroyed
Note: See TracTickets for help on using tickets.