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

Do not cache user-initiated Refresh Status requests from the client #15154

Closed
Miyayes opened this issue Apr 6, 2021 · 6 comments · Fixed by brave/brave-core#8533
Closed

Do not cache user-initiated Refresh Status requests from the client #15154

Miyayes opened this issue Apr 6, 2021 · 6 comments · Fixed by brave/brave-core#8533

Comments

@Miyayes
Copy link
Collaborator

Miyayes commented Apr 6, 2021

Description

When a user presses "Refresh Status" to get the latest information on a Creator/publisher, this call should never be client-cached so as to always fetch the latest Creator information from the PCDN.

Solution

The network request from the client should always skip any sort of client-side caching. This should be implemented in the browser code.

@Miyayes Miyayes added OS/Android Fixes related to Android browser functionality OS/Desktop feature/rewards labels Apr 6, 2021
@emerick
Copy link
Contributor

emerick commented Apr 6, 2021

We'll want to modify our UrlRequest structure to have a new load_flags field and update RewardsServiceImpl::LoadURL to pass it through to the network::ResourceRequest. Then need to update the url request in GetPublisher::Request to send the appropriate flag(s) (probably net::LOAD_BYPASS_CACHE or something similar).

@yachtcaptain23
Copy link

I could be mistaken, but I don't think there's client caching judging by the call chain of RewardsServiceImpl.RefreshPublisher which eventually calls vendor/bat-native-ledger...publisher.cc's RefreshPublisher and then to ServerPublisherFetcher. It seems during normal cases the "Refresh Status" will always trigger a network call.

There's possibly a couple of hypothetical edge cases, which might cause this erroneous state we saw, so I think we should investigate further or add instrumentation to help with diagnosis

@emerick
Copy link
Contributor

emerick commented Apr 9, 2021

@yachtcaptain23 Yeah, we don't cache the request in the client; it's the Chromium network layer that's caching it. You can see this if you send a request, disconnect your Internet, and send another request. Even with the Internet disconnected, Brave will behave as though both requests succeeded. I believe this is happening via this class, but not positive: https://source.chromium.org/chromium/chromium/src/+/master:net/http/http_cache.h.

Setting net::LOAD_BYPASS_CACHE or a similar flag (or combination of flags) in the ResourceRequest should bypass this caching. We do something similar in the stats updater, from what I recall.

@stephendonner
Copy link

Verified PASSED using the testplan from brave/brave-core#8533 with build

Brave 1.25.28 Chromium: 90.0.4430.72 (Official Build) nightly (x86_64)
Revision b6172ef8d07ef486489a4b11b66b2eaeed50d132-refs/branch-heads/4430@{#1233}
OS macOS Version 11.2.3 (Build 20D91)

Steps:

  1. new profile
  2. launched Brave
  3. enabled Rewards
  4. loaded yachtcaptain23.github.io, clicked on the BAT icon, then clicked Refresh Status
  5. confirmed I saw Brave Verified Creator in the UI, and an HTTP 200 in the logs for the GET request
  6. disconnected Wi-Fi
  7. clicked on the BAT icon, clicked Refresh Status
  8. confirmed I got the following in the logs, and the status in the UI said Not yet verified:
[ REQUEST ]
> URL: https://pcdn.bravesoftware.com/publishers/prefixes/7018
> Method: UrlMethod::GET
[19304:775:0420/171058.862830:VERBOSE6:logging_util.cc(136)] 
[ RESPONSE - OnRequest ]
> Url: https://pcdn.bravesoftware.com/publishers/prefixes/7018
> Result: Error (net::ERR_INTERNET_DISCONNECTED)
> HTTP Code: -1
  1. reconnected to Wi-Fi, clicked the BAT icon, clicked Refresh Status
  2. confirmed I saw the Brave Verified Creator status and in the logs saw:
[ REQUEST ]
> URL: https://pcdn.bravesoftware.com/publishers/prefixes/7018
> Method: UrlMethod::GET
[19304:775:0420/171509.755365:VERBOSE6:logging_util.cc(136)] 
[ RESPONSE - OnRequest ]
> Url: https://pcdn.bravesoftware.com/publishers/prefixes/7018
> Result: Success
> HTTP Code: 200
Wi-Fi connected; verified Wi-Fi disconnected; unverified Wi-Fi reconnected; verified
Screen Shot 2021-04-20 at 5 10 22 PM Screen Shot 2021-04-20 at 5 11 04 PM Screen Shot 2021-04-20 at 5 11 27 PM

@stephendonner stephendonner added QA Pass-macOS and removed QA/In-Progress Indicates that QA is currently in progress for that particular issue labels Apr 21, 2021
@srirambv
Copy link
Contributor

srirambv commented May 19, 2021

Removing Android label as there is no refresh button on Android once the publisher status is updated. Follow up issue #15966 for Android

@srirambv srirambv removed the OS/Android Fixes related to Android browser functionality label May 19, 2021
@Miyayes
Copy link
Collaborator Author

Miyayes commented Jul 15, 2021

Do we know if the above fix would also apply to Android? That is, if the user presses "refresh status" on Android, it will always bypass local cache?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment