Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

No adblock when a tab is reopened from "Recent tabs"? #681

Closed
elcste opened this issue Aug 10, 2020 · 5 comments · Fixed by #705
Closed

No adblock when a tab is reopened from "Recent tabs"? #681

elcste opened this issue Aug 10, 2020 · 5 comments · Fixed by #705
Labels

Comments

@elcste
Copy link

elcste commented Aug 10, 2020

Bromite version

Version: 84.0.4147.121
Arch: arm
Android version: 10
Device model: Nokia 1

Is this bug about the SystemWebView?

No

Is the bug reproducible with latest version?

Yes

Can the bug be reproduced with corresponding Chromium version?

N/A: Bromite adblock issue

Is the bug a crash?

No

Describe the bug

When I reopen tabs from the "Recent tabs" menu option, those tabs show ads that are blocked if I navigate to the same page in a different way or just reload the page. I've reproduced on a few sites:

Steps to reproduce the bug

Steps to reproduce the bug:

  1. Visit a page with ads, such as one of the ones listed above.
  2. See there are no ads when adblock is enabled.
  3. Close the tab.
  4. In another tab, click on the menu, then "Recent tabs", then the tab you just closed.
  5. Observe that the reopened tab does show ads that were previously blocked.
  6. Reload the page. Observe that ads again are blocked.

Expected behavior

Tabs reopened from "Recent tabs" should have ads blocked like a page opened any other way.

@csagan5 csagan5 added the bug label Aug 14, 2020
@csagan5
Copy link
Contributor

csagan5 commented Aug 19, 2020

It could be related to the subresource filter not being added in such situation.

Can the bug be reproduced with corresponding Chromium version?

N/A: Bromite adblock issue

The ad blocker is in fact present also in Chromium, so this could be tested also there - although it uses a very specific default set of ads to block.

@elcste
Copy link
Author

elcste commented Aug 20, 2020

The ad blocker is in fact present also in Chromium, so this could be tested also there - although it uses a very specific default set of ads to block.

I thought about that after I posed the issue. If you have any idea how to find a site to test with, I will. I poked around the Chromium source, but everything just said things like what is blocked are ads that violate the "Better Ads Standard", without any block list that I could locate.

@uazo
Copy link
Collaborator

uazo commented Aug 23, 2020

I found that bug is related to the fact that the subframe (in ContentSubresourceFilterThrottleManager::MaybeCreateActivationStateComputingThrottle) does not have the ActivationState of the parent mainframe.

In fact, I found that the web_contents created in https://source.chromium.org/chromium/chromium/src/+/master:chrome/browser/sessions/session_restore_android.cc;l=35 did not have connect ChromeSubresourceFilterClient.

This - I think - is given by NavigationRequest::StartNavigation() launches navigation of a web_contents without the correct delegate (here https://source.chromium.org/chromium/chromium/src/+/master:content/browser/frame_host/navigation_throttle_runner.cc;l=99) and then generates NULL in https://source.chromium.org/chromium/chromium/src/+/master:chrome/browser/chrome_content_browser_client.cc;l=4041

it happens because (chrome/browser/android/tab_android.cc).AttachTabHelpers is not called before the SessionRestore::RestoreForeignSessionTab() and therefore the NavigationThrottleRunner::RegisterNavigationThrottles() doesn't have the ability to invoke ChromeSubresourceFilterClient::MaybeAppendNavigationThrottles (that insert the adblocker control).

Now, the definitive solution would be to try to generate a suitable web_contents_android, but I would have to replicate AttachTabHelpers() handling in RestoreForeignSessionTab, absurd to maintain. maybe there is some other cleaner way too, but I haven't found it.
I worked around the bug by making future maintenance easier, with only one edited file: restoring tab from recently closed tabs, java will create navigation in a new tab, closing the open one. from the point of view of the ui it is more or less identical.

I think that the bugs does not exist in upstream because some parts of code are called by the safe_browsing component, in bromite doesn't exits.

@csagan5
Copy link
Contributor

csagan5 commented Aug 23, 2020

Very nice analysis, thanks @uazo. Is it possible that there are other bugs upstream because of this asymmetry? If you find a security bug - present upstream even with the safe browsing component - then you could submit it, claim a bounty and see it fixed relatively quickly. We could also check if GrapheneOS/Vanadium is affected.

For the time being the PR you made should suffice.

@uazo
Copy link
Collaborator

uazo commented Aug 23, 2020

from what I've seen, all calls to SwapWebContents
https://source.chromium.org/chromium/chromium/src/+/master:chrome/browser/android/tab_android.h;l=101 could have that problem, today only restore and webportal (which I don't know still what it is).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants