Non SSL errors are affected too, previous Canary (3332?) was OK

Deleted: Screenshot_20180201-173224.png 157 KB

	Screenshot_20180201-173224.png 157 KB View Download

edwardjung could you help triage this please? Looks like it's Android only.

As far as I can debug, the fonts are not being specified on Android so default fallbacks are being used.

It affects all Web UI pages under chrome://interstitials and chrome://neterrors. 

webui::SetLoadTimeDataDefaults() is returning blank for the fontSize and fontFamily
https://cs.chromium.org/chromium/src/ui/base/webui/web_ui_util.cc?type=cs&l=171

The values are also not showing in chrome://resources/css/text_defaults.css template. See the attached screen shots and look at the elements / sources panels.

Not sure who to assign it to, needs a Android engineer who is familiar with the Android stack as it seems isolated to that platform. 

Deleted: Screen Shot 2018-02-01 at 19.51.01.png 700 KB

	Screen Shot 2018-02-01 at 19.51.01.png 700 KB View Download

Deleted: Screen Shot 2018-02-01 at 19.51.20.png 305 KB

	Screen Shot 2018-02-01 at 19.51.20.png 305 KB View Download

Similar case: https://bugs.chromium.org/p/chromium/issues/detail?id=810507 (but PLEASE do not mark as duplicate - reason in 810507)

I can't reproduce this on the latest Canary 66.0.3343.0 on a Nexus 5.

Can you confirm this is still happening for you on the latest Canary? If so we might need to track down an S7 and see if it repros.

Still visible on official Canary 66.0.3343.0, S7, Android 7

Thanks for confirming. I located a Samsung Galaxy S6, and this is definitely a regression between dev 66.0.3335.4 and Canary 66.0.3342.3 on that device. Looks like it won't repro on Nexus devices, and will repro minimally on S6 and S7s (possibly others). Screenshots attached of dev and canary on that device.

The changelog is at https://chromium.googlesource.com/chromium/src/+log/66.0.3335.0..66.0.3342.0?pretty=fuller&n=10000. I didn't see anything too obvious in WebUI or Android... but it's a big changelog and I'm not 100% sure what to look for.

+Android folks for help in triaging, upgrading to ReleaseBlock-Dev

Deleted: Screenshot_20180209-135702.png 243 KB

	Screenshot_20180209-135702.png 243 KB View Download

Deleted: Screenshot_20180209-135654.png 246 KB

	Screenshot_20180209-135654.png 246 KB View Download

+WebUI folks as well.

Issue 810507 has been merged into this issue.

Attached what the phishing interstitial looks like.

Deleted: Screenshot_20180209-141755.png 79.2 KB

	Screenshot_20180209-141755.png 79.2 KB View Download

#13: to clarify, this is the same Galaxy S6 used for the screenshots in #10, running the same Canary 66.0.3342.3.

A narrower revision range is https://chromium.googlesource.com/chromium/src/+log/66.0.3335.0..66.0.3336.0?pretty=fuller&n=10000, using the originally reported bad version of 66.0.3336.0 from the first comment. I took a look through there and didn't see anything immediately relevant either.

Whoops, mistaken label

This is also happening on on a Pixel 2, which the screenshots in #5 are from. Still happening on canary 66.0.3343

Are there consistent repro steps? If so, let's get clank-te@ to bisect.

Re:#17 
Going to any of the urls on chrome://interstitials you will see the messed up fonts.

The other test is going to:
 chrome://resources/css/text_defaults.css 

You'll see that the the font-family and font-size values are empty.

+ It seems strange that the Nexus 5 isn't affected. 
+ The safe browsing interstitial is the exception as it looks like all the string values aren't getting populated at all, which might / might not be related.

Deleted: Screenshot_20180209-165509.png 217 KB

	Screenshot_20180209-165509.png 217 KB View Download

The values are supposed to be swapped in at run-time on the fly, when pulling the css file from disk and serving it to WebUI, see [1]. It seems that something related to this replacement is broken in that flow.

cc'ing dschuyler@ in case they are aware of any change that could have affected this.

[1] https://cs.chromium.org/chromium/src/ui/webui/resources/css/text_defaults.css?type=cs&q=text_defaults.css+-file:third_party+-file:infra&l=18-29

The $i18n{<key>} values should be replaced. If <key> were not found it should report a CHECK failure at https://cs.chromium.org/chromium/src/ui/base/template_expressions.cc?type=cs&sq=package:chromium&l=97

If it's not reporting a CHECK failure then maybe look into whether "fontFamily" and other) are being set to an empty string (which would be unusual).

Iff the run-time file contained lines like:
  font-family: $i18nRaw{fontFamily};
Then I'd suspect that ReplaceTemplateExpressions() wasn't being called, *but* #18 (above) suggests that they are empty -- so something must be processing the $i18n tags.

Suggestion: add some tracing in (or step in the debugger in) ReplaceTemplateExpressions():
https://cs.chromium.org/chromium/src/ui/base/template_expressions.cc?type=cs&sq=package:chromium&l=61

FYI, I'm not familiar with the Android builds per se. I'd prefer to have a Linux repro; or assist an Android SWE*. 


*Feel free to send me chat, email, or responses in this bug (i.e. I'm happy to help).

ramine, can you try to bisect?

Bisect Info:

Good build:  66.0.3335.4
Bad build:   66.0.3336.0
Regression range: https://chromium.googlesource.com/chromium/src/+log/66.0.3335.4..66.0.3336.0?pretty=fuller&n=10000

Good commit: 533408
Bad commit: 533409
suspect CL:
https://chromium.googlesource.com/chromium/src/+/f9c9c93a13722b535707412b677f2cfb187fd17c

Is this CL the issue? 

sadrul@, can you please take a look if your CL could be related?

The CL (crrev.com/533409) is unrelated.

From the CL range, I don't see anything immediately obvious, but it is fairly big.

The code that replaces the fontSize,fontFamily,textDirection is at [1] and does not seem to have been touched recently. Also the fact that in the screenshot of comment #18 the textDirection is present, and only font-family and font-size are missing, indicates that the file is actually being processed, but empty values are provided, which I have no explanation since that code seems also not affected for example see [2].

Either way, there are quite a few Android specific CLs in the range (and a few font related, as well as "locale" related). I suggest that someone with a local Android build do a build-bisection (try reverting CLs one by one) to narrow it down.


[1] https://cs.chromium.org/chromium/src/ui/base/webui/web_ui_util.cc?type=cs&q=%5C%22fontFamily%5C%22&l=187-193
[2] https://cs.chromium.org/chromium/src/ui/base/webui/web_ui_util.cc?type=cs&l=229

Bisect info:

Good build:66.0.3335.4
Bad build:66.0.3337.0
Regression range:
https://chromium.googlesource.com/chromium/src/+log/66.0.3335.0..66.0.3337.0?pretty=fuller&n=10000

Good commit:533440
Bad commit:533441
Suspect CL:
https://chromium.googlesource.com/chromium/src/+/4db4cd30a2de164262978718c75dd2cfd7f73235

Assigning to reviewer estark@-Could you please take a look,Since unable to assign it to the marcin@-CL author.

The CL https://chromium.googlesource.com/chromium/src/+/4db4cd30a2de164262978718c75dd2cfd7f73235 isn't the culprit because the bug was first spotted in a build before that commit (see original report).

tedchoc, is there anyone who works on Clank regularly who might be able to debug this?

Note to #27: this is fully incorrect,

1. we didn't touch propagating fonts with this fix

2. https://bugs.chromium.org/p/chromium/issues/detail?id=806609#c7 (problem was visible before this fix was in the code, thx estark@ for notifying it)

3. I've just build Chromium with this change and without this change and in both cases fonts are OK, problem is in Google build for Chrome!

4. > Assigning to reviewer estark@-Could you please take a look,
> Since unable to assign it to the marcin@-CL author.

Why unable?

Btw, I suspect the repro is flaky because different regression ranges have been reported and the bug was observed with and without 4db4cd30a2de164262978718c75dd2cfd7f73235.

Interstitials are not an area owned by chrome Android engineers.  If we think it is a systemic font issue, then the blink font owners are going to be a better candidate than Android ones.

Note this isn't really interstitials, this is showing up most prominently on net errors. The Safe Browsing interstitial problem does seem to be related though. I have a Canary and a Chromium build on my device at the exact same revision (c8ce2d88ee3), and both problems (weird net error fonts, missing text on interstitials) occur on Canary but not on the Chromium build. (As noted in #29, might be a problem with official builds.)

Can't really repro on an official build... maybe a field trial is causing it?

I'm sorry but I'm really not the right person to look into this. This is not at all related to interstitials, but rather seems to affect all sorts of WebUI on Android (e.g. chrome://version).

For some reason the resource ids 26903 (IDS_WEB_FONT_FAMILY) and 26904 (IDS_WEB_FONT_SIZE) don't appear to be present; I see "[WARNING:resource_bundle.cc(605)] unable to find resource: 26903" in the logs on the Canary (66.0.3345.0) on which I can repro.

Doesn't seem to have to do with any field trials, nor official vs unofficial build (can't repro with a locally built official build).

sky, does this seem at all possibly related to https://chromium.googlesource.com/chromium/src/+/a3aee722458ee66ade96e5dad31878e3570dea8f? Maybe some kind of race introduced by that change?

Great investigation - I don't see any better candidates in the regression range. Since this is pretty serious (all the red interstitials look exactly like one another, various other WebUI pages with serious visual regressions), I'd suggest a speculative revert to see if that fixes the issue. It's a big CL, but hopefully it's still soon enough to revert cleanly.....

At this point it would be rather hard to revert that patch. I'll look ASAP. Does this only impact Android?

If it is android, could you suggest steps to reproduce locally? If you say I should deploy to android that's fine, I'll figure out how to do that.

It does appear to impact only Android, though the repro is rather finicky so it's hard to say for sure that it's Android-only. Theoretically, one should be able to reproduce by building for Android, installing on a device, and navigating to http://asdfasdfasfsd.test or any other page that generates a net error. You can also observe the messed-up font on chrome://version. If it repros, it'll generate the warning log message in #34. However, I haven't been able to repro on any local build, only on Canary 66.0.3346.0. That suggests to me that it is either a race, or a problem with the build process. (Note that I wasn't able to repro by building an official build either, so I lean towards the former.)

tl;dr; Verified: Missing resources can result in empty i18n strings.

Details:

I was curious if not finding the resource would result in an empty string being passed to the $i18n{} replacement code mentioned in #20, above.

#34 points out the resource IDs and code at resource_bundle.cc(605). That makes it easy to see that at resource_bundle.cc(607):  return base::string16(); will create an empty string for the replacement.

I further verified by putting
  if (message_id == 26903)
    return base::string16(base::ASCIIToUTF16("bogus-font-name"));
at resource_bundle.cc(589), which will use "bogus-font-name" for the font-family in chrome://resources/css/text_defaults.css

 

I noticed that failure to load resources is not a hard error. Specifically LoadFromApkOrFile() in resource_bundle_android.cc. That seems rather surprising as it leads to weird errors like this.

The patch estark@ mentions above moved loading of resources earlier, so if anything I would expect that to make a race harder to tickle. Unless there is some state that needs to be installed before base::android::OpenApkAsset.

What's the easiest way to repro?

See #38 for repro instructions.

Also adding in tools/grit OWNERS in case they have ideas.

I can't repro this on a 6P on a tip of tree build. I tried going to the first interstitial in chrome://interstitials as well as chrome://resources/css/text_defaults.css . I will try a pixel next.

I'm starting chromium, which opens the NTP, and then typing in these urls. Is that a good repro? Should I instead by launching chrome so that it only opens these pages?

I think you need a branded build, as no one has been able to reproduce this with a local developer build so far. In particular if we think that this has something to do with field trials, they might depend on the Finch seed.

Bernhard suggested I likely need an official channel too. I'm going to try

is_chrome_branded=true
android_channel = canary

Re #44: I wasn't able to repro on a is_chrome_branded=true build. I also don't think it's a Finch trial because prashanthpola@ has a repro with only the single variation 5e3a236d-59e286d0 (enables crash reporting) in chrome://versions.

I'm wfh today and don't have an Android device handy, but there's a new Canary out today, 66.0.3347.0. Could somebody try reproing on that? (The actual build being served on the Canary channel, not a local build of it.)

> I'm wfh today and don't have an Android device handy, 
> but there's a new Canary out today, 66.0.3347.0.
> Could somebody try reproing on that? 
> (The actual build being served on the Canary channel, 
> not a local build of it.)

if you ask about checking if bug is there, then answer is: yes, it is

I couldn't repro on a local canary build either, but I could repro on latest canary downloaded from store (66.0.3347.0). Any suggestions as to what differs between the two? I did click the send crash feedback button.

Re #48: thanks

grit owners: I assume that the relevant string is supposed to end up in a pak file somewhere in the apk. Do you know if there is a way to tell which file that should be and unpack it to check if the string is actually there in the repro-ing apk? That would help narrow down whether it's a problem in the build process (string isn't ending up in the apk) or in the process of loading the resource.

Make sure to build with is_official_build=true
That enables the .pak dead-resource-removal code (and also outputs a *lot* of build-spam)

For debugging the build - you can use //tools/grit/pak_util.py to inspect .pak files.
If you have a local build, you can find the IDS_->numeric ID mapping in //out/Release/foo.pak.info

E.g.: From my local is_official_build:

$ grep IDS_WEB_FONT out-gn/Release/locales/en-US.pak.info
IDS_WEB_FONT_FAMILY,26903,../../ui/strings/app_locale_settings.grd
IDS_WEB_FONT_SIZE,26904,../../ui/strings/app_locale_settings.grd

$ tools/grit/pak_util.py print out-gn/Release/locales/en-US.pak | grep 2690
Entry(id=26903, canonical_id=26903, size=17, sha1=0e0df27538): Arial, sans-serif
Entry(id=26904, canonical_id=26904, size=3, sha1=b77a111bfd): 75%

From the reports, sounds like it's also a good idea to be testing with monochrome_apk. This is the version of the apk we send to devices running android N+. In Monochrome.apk, locale paks are split into two:

$ unzip -l apks/Monochrome.apk |grep en-US.pak
    49944  2001-01-01 00:00   assets/en-US.pak
     6992  2001-01-01 00:00   assets/stored-locales/en-US.pak

In this case, it looks like the string is there in the second file:
tools/grit/pak_util.py print out-gn/Release/assets/stored-locales/en-US.pak | grep "sans-serif"
Entry(id=26903, canonical_id=26903, size=17, sha1=0e0df27538): Arial, sans-serif

Sounds like the commit in question was re-ordering start-up code. One thing to check, is whether that this line must be run in order to enable loading of the second .pak file for monochrome:

https://cs.chromium.org/chromium/src/chrome/browser/chrome_browser_main_android.cc?rcl=cfb4e414ce32e8b7cdf506ec6ce21f9d83628a33&l=45

agrieve, I bet that is it! I suspect that needs to move to PreEarlyInitialization. Any idea why this would only be reproducible when downloading from play store vs locally?

That logic is only relevant for Monochrome.apk. So it could be that locally no one has been building monochrome_apk.

As an aside - unless we expect to be using different fonts for different languages, we shouldn't be putting these strings in locale .pak files, since that will mean the same string will be copied into every single locale .pak file.

#55 (on aside) I believe we do, here's a search that shows some variance:
https://cs.chromium.org/search/?q=%5C%22IDS_WEB_FONT_FAMILY%5C%22+file:%5C.xtb$&type=cs

 

Patch up for review here: https://chromium-review.googlesource.com/c/chromium/src/+/919969

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

commit 2b8d180e2b74fc9086497ec79c39fa378509095a
Author: Scott Violet <sky@chromium.org>
Date: Thu Feb 15 16:58:24 2018

android: fix loading of secondary locale paks

In certain situations (mono-chrome) Android may load secondary locale
paks. This is done in
ChromeBrowserMainPartsAndroid::PreCreateThreads(). This registration
must happen before ui::ResourceBundle is created(). As part of moving
loading local state earlier ResourceBundle is now created at an
earlier time than PreCreateThreads(). This means the secondary locale
registration effectively has no effect and breaks resources for
mono-chrome.

The fix is to move registration to PreEarlyInitialization(), before
ChromeBrowserMainParts creates ResourceBundle.

BUG= 808049 
TEST=none

Change-Id: I6f987d7dc88bed387ee999dc4a9a147a5960398e
Reviewed-on: https://chromium-review.googlesource.com/919969
Reviewed-by: Ted Choc <tedchoc@chromium.org>
Reviewed-by: agrieve <agrieve@chromium.org>
Commit-Queue: Scott Violet <sky@chromium.org>
Cr-Commit-Position: refs/heads/master@{#537050}
[modify] https://crrev.com/2b8d180e2b74fc9086497ec79c39fa378509095a/chrome/browser/chrome_browser_main_android.cc