Using the per-revision bisect providing the bisect results,
Good build: 54.0.2791.0 (Revision: 404283).
Bad build: 54.0.2793.0 (Revision: 404564).

You are probably looking for a change made after 404320 (known good), but no later than 404321 (first known bad).
CHANGELOG URL:
  https://chromium.googlesource.com/chromium/src/+log/bace6ed0fd91cd357569dc9ad1d285ff2390520e..7ae0fd5c2969a7253c23fdad6993898323aba694

@toyoshim -- Could you please look into the issue, pardon me if it has nothing to do with your changes and if possible please assign it to concern owner.

Thank You.

stevenjb@ can you take a look?

My CL changed a reload behavior not to force revalidating sub-resources.
Since I do not understand how NTP work, I have no idea what is the best fix here.

When I change the search engine, existing NTP pages seem to be reloaded automatically. But it could not pick up contents for newly specified search engine.

FYI, newly opened NTP shows new search engine correctly.

Set NTP label.

The relevant NTP code is in BrowserInstantController::DefaultSearchProviderChanged, in particular here:
https://cs.chromium.org/chromium/src/chrome/browser/ui/browser_instant_controller.cc?rcl=0&l=192

Not sure why that would be affected by the change above, but maybe this helps someone else figure it out :)

I notice that this happens even on chrome://settings.
Also, it's a little racy. Sometime it reloads correctly, and sometime it doesn't.
So probably, I need a help from NTP team.

One possible reason is that my change makes reload faster than before, and it loads something before something is ready to reflect search engine updates.

> One possible reason is
Ah, this isn't possible because when I saw it does not reload correctly, manual reloads do not work at all.

I can be the person on the NTP side, but I know nothing about this reload change. Could you take a look at the code I linked above and see if anything there looks fishy to you? Thanks!

Before investigating actual code, I have some questions about the NTP implementation.

- How is the main document provided?
- How is the search box content provided? If provided in a different resource, what resource URI is?

If the search box isn't part of the main document, memory cache or disk cache would cache the previous content and loading follows the cache control protocol.

Thanks for looking into this!

The main document is a web page served from https://www.google.com/_/chrome/newtab (cached by a ServiceWorker, but that shouldn't matter).

I'm not sure what you mean by "search box content"?

I mean Google search box in the NTP that is shown only when Google is selected as a default search engine.

Ah - that's a regular part of the web page. It's actually not really a search box at all; when you start typing, it just redirects focus to the Omnibox. I don't think that's related to the problem.

Some background on the reload logic:
For NTPs, Chrome provides some special Javascript APIs (such as the focus mangling above). What's considered an NTP depends on the chosen search engine, so when the search engine changes, then what used to be an NTP isn't an NTP anymore. That's what the reload in BrowserInstantController is for: We load the page again, the page detects that the special NTP APIs aren't there anymore, and so it redirects to a standard Google home page.

Thanks, that information would help me to investigate.
But, sorry time is already up today. I'd investigate this next week.

On reload, chrome-search://local-ntp/local-ntp.html is loaded that includes two scripts in the head tag.

<script src="chrome-search://local-ntp/config.js"></script>
<script src="chrome-search://local-ntp/local-ntp.js"></script>

But after my reload change, these scripts are served from memory cache in Blink.

As a result, config.js contains old configData that says { "isGooglePage": true } even after changing the search engine to a different one.

This is what I said, sub-resource loading should follow cache protocol. To avoid this problem, if these are served by http(s), it needs to return cache control headers, ETag, and so on correctly. Or using different URI for different contents will solve the problem.

Thanks for the investigation!
Using a different URI won't work; the whole point of that config.js is to pass options to the page. Setting cache headers should be possible though.

I'm a bit confused about the "http(s)" comment - these scripts are served from chrome-search://, not http(s). But I guess at that cache layer in Blink, that amounts to the same thing?

+dbeam

OK, I have a fix. I'd send a CL for review soon.

https://cs.chromium.org/chromium/src/content/browser/webui/web_ui_data_source_impl.cc?sq=package:chromium&dr=CSs&rcl=1480378899&l=60

just override AllowCaching to return false in LocalNtpSource

https://cs.chromium.org/chromium/src/chrome/browser/search/local_ntp_source.h?q=localntpsource&sq=package:chromium&l=16&dr=CSs

Yeah, but is it ok to return false for all resources in terms of performance impact? If there is a concern, I'd split non-cachable resources into LocalNtpNonCacheSource.

i'll let treib@ answer that definitively, but I assume it's OK because fairly little is being done to generate the contents of those URLs:
https://cs.chromium.org/chromium/src/chrome/browser/search/local_ntp_source.cc?sq=package:chromium&dr=CSs&rcl=1480378899&l=149,169

Yeah, caching in Blink or just getting the files from LocalNtpSource again shouldn't make much of a difference in terms of performance. I've also tried it locally and there doesn't seem to be a big difference (though the timings are somewhat noisy and it's hard to judge). I'll keep an eye on the UMA metrics after the change lands, but let's assume it's all good.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/501ba310da88bd1b84202f3f4c9b38cc5e6ea899

commit 501ba310da88bd1b84202f3f4c9b38cc5e6ea899
Author: toyoshim <toyoshim@chromium.org>
Date: Wed Nov 30 08:25:55 2016

New Tab Page: chrome-search://local-ntp/ files should not be cachable

Under chrome-search://local-ntp/, there is a file that can be modified
dynamically, e.g., config.js, and such a file should not be cachable
so that reload operations can load new data correctly.

This wasn't a problem before because Chrome's reload revalidates all
resources forcibly, but recent change made a normal reload revalidate
not all-resources, but only main resource.

BUG= 668454 

Review-Url: https://codereview.chromium.org/2533203002
Cr-Commit-Position: refs/heads/master@{#435166}

[modify] https://crrev.com/501ba310da88bd1b84202f3f4c9b38cc5e6ea899/chrome/browser/search/local_ntp_source.cc
[modify] https://crrev.com/501ba310da88bd1b84202f3f4c9b38cc5e6ea899/chrome/browser/search/local_ntp_source.h