1733490 - Migrate CSPValidator::FormatErrorParams to Fluent

Status: RESOLVED FIXED

(WebExtensions :: General, task, P3)

Description

[Zibi Braniecki [:zbraniecki][:gandalf]]

https://searchfox.org/mozilla-central/rev/7539ad54ddc720a0553efd07ca681b9a409f9887/toolkit/mozapps/extensions/AddonContentPolicy.cpp#359 uses StringBundle.

We should be able to migrate the 6 strings listed here: https://searchfox.org/mozilla-central/search?q=csp.error.&path=&case=false&regexp=false

to Fluent and use C++ Localization class instead.

Comment 1

[Tomislav Jovanovic :zombie]

Changing severity to NA because it's a task.

Comment 2

[Eemeli Aro [:eemeli]]

Zibi, where should this .ftl file go? I'm not seeing any other C++ files around there yet using Fluent.

Edit: Also, is there an example available somewhere for the right way of constructing the dom::Record object that e.g. FormatValue requires for parameters? All of the C++ code calling any of the Localization formatting methods appear to not use any parameters, and my m-c grep-fu has not revealed anywhere in the code that a Record is created without coming in via the IDL.

Comment 3

[Zibi Braniecki [:zbraniecki][:gandalf]]

I'd assume you want to place it in https://searchfox.org/mozilla-central/source/toolkit/locales/en-US/toolkit/global to match where it is right now.

Also, is there an example available somewhere for the right way of constructing the dom::Record object that e.g. FormatValue requires for parameters?

L10nArgs l10nArgs;

auto arg = l10nArgs.Entries().AppendElement();
arg->mKey = "userName";
arg->mValue = "John";

Comment 4

[Eemeli Aro [:eemeli]]

Attachment: Bug 1733490 - Migrate CSP error messages from extensions.properties to Fluent. r=zbraniecki,mixedpuppy

Comment 5

[Eemeli Aro [:eemeli]]

Started a patch for this, which at least initially seems to work as intended, but I have a couple of questions still about the C++ code:

1. Is there a more ergonomic way of constructing the l10nArgs argument for FormatValueSync?
2. Does it make sense to continue keeping separate FormatError and FormatErrorParams methods? Previously, the former was a template function that accepted a variable number of arguments, but I've here specialised it to just one or three, as that's what the code actually requires.
3. Should there be some memoization of the Localization instance?

Finally, on a more fundamental level, why is this in Fluent M2? Is there some CSP error that ends up always formatted during startup, or is it a matter of there just being a StringBundle reference anywhere in AddonContentPolicy?

Comment 6

[Zibi Braniecki [:zbraniecki][:gandalf]]

Finally, on a more fundamental level, why is this in Fluent M2? Is there some CSP error that ends up always formatted during startup,

Yes, we see four calls to that during startup.

You can see it if you go to https://www.arewefluentyet.com/list.html?milestone=M2 and filter for "csp.error.m"

Now, is it a good thing that we're doing it during startup? Should we? Is it some hidden bug? And if we do, should we memoize Localization to avoid constructing it 4 times during startup? [0]
For those, and other fascinating questions, I'mma gonna summon :mixedpuppy :)

[0] Localization constructor is actually quite cheap, as it is just a vector of cached resources, but some message id hashing during construction is still done so maybe worth memoizing?

Also, filed bug 1742106 to add some basic tests for boilerplate C++ API for l10nArgs.

Comment 7

[Dan Minor [:dminor]]

(In reply to Eemeli Aro [:eemeli] from comment #5)

Started a patch for this, which at least initially seems to work as intended, but I have a couple of questions still about the C++ code:

1. Is there a more ergonomic way of constructing the l10nArgs argument for FormatValueSync?

It doesn't look like it, it looks like if you try to use dom::Optional(l10nArgs) as an argument to FormatValueSync it tries to make a copy, and the copy constructor for l10nArgs is deleted.

2. Does it make sense to continue keeping separate FormatError and FormatErrorParams methods? Previously, the former was a template function that accepted a variable number of arguments, but I've here specialised it to just one or three, as that's what the code actually requires.

I'd suggest just combining the methods.

3. Should there be some memoization of the Localization instance?

I'll defer to Zibi and :mixedpuppy on this one.

Comment 8

[Eemeli Aro [:eemeli]]

WIP patch updated to address the above concerns, except for any explicit memoization.

Zibi, it looks like the formatting that's done during startup may well be superfluously eager. I've put together a fix that makes it lazier by only formatting the error if it's actually needed. Two questions related to that:

1. As it's a somewhat separate change from moving the messages from .properties to Fluent, should I just include it in my current patch, make it a separate patch on this same bug, or do I need to file a separate bug about it?
2. How can I validate that my change has an effect on M2?

Comment 9

[Zibi Braniecki [:zbraniecki][:gandalf]]

As it's a somewhat separate change from moving the messages from .properties to Fluent, should I just include it in my current patch, make it a separate patch on this same bug, or do I need to file a separate bug about it?

The best route would be to introduce a patchset (not sure how git cinnabar handles that, but if you use HG it would be a bookmark with two patches), where one patch moves logic around, and the other switches Properties to Fluent.

This would allow each patch to be reviewed by the appropriate reviewers.

How can I validate that my change has an effect on M2?

M2 is the most tricky milestone to collect data for because I requires full instrumented Firefox build.

We take the patch from bug 1501881, apply it on m-c, rebuild Firefox, and launch it.
This dumps log from the runtime, which AWFY script collects - https://github.com/projectfluent/arewefluentyet.com/blob/master/src/arewefluentyet/milestone2.py

If you want to do it yourself, you can either:

1. Apply the patch, rebuild Firefox, run, and interpret log yourself
2. Learn how to use awfy script to do ./aggregate.py --mc ../mozilla-central -m M2

If you want to try, ping Erik or me on Matrix and we'll walk you through.

Comment 10

[Eemeli Aro [:eemeli]]

Attachment: Bug 1733490 - Be lazier about formatting missing-directive CSP error. r=mixedpuppy

Pre-emptively formatting the error in the constructor is a bit
wasteful, and happens four times during startup with a fresh profile.

Let's instead do the formatting only if the error actually happens.

Depends on D131594

Comment 11

[Eemeli Aro [:eemeli]]

I got the M2 logging set up, and was able to verify that the lazy formatting works as intended. Also, submitting the changes as separate patches seems to work exactly like it ought to.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f5bcc7acaf26
Migrate CSP error messages from extensions.properties to Fluent. r=zbraniecki,mixedpuppy,flod

Comment 13

[Norisz Fay [:noriszfay]]

Backed out for causing mochitest failures on browser_all_files_referenced.js

Backout link: https://hg.mozilla.org/integration/autoland/rev/e7244417f6170709efd864711dfaa0735d79f540

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel&revision=f5bcc7acaf26ddd203f07520a0b86b1081954a20&selectedTaskRun=GS6dh6s6TDmv4wlK5ZDxVg.0

Failure log: https://treeherder.mozilla.org/logviewer?job_id=361162474&repo=autoland&lineNumber=3050

Failure line: TEST-UNEXPECTED-FAIL | browser/base/content/test/static/browser_all_files_referenced.js | there should be no unreferenced files - Got 1, expected +0

Comment 14

[Eemeli Aro [:eemeli]]

Added a whitelisting to the test for the added FTL file, as it's only used from a C++ file.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e5f576264d50
Migrate CSP error messages from extensions.properties to Fluent. r=zbraniecki,mixedpuppy,flod
https://hg.mozilla.org/integration/autoland/rev/6e1efc745be9
Be lazier about formatting missing-directive CSP error. r=mixedpuppy

Comment 16

[Marian-Vasile Laza]

https://hg.mozilla.org/mozilla-central/rev/e5f576264d50
https://hg.mozilla.org/mozilla-central/rev/6e1efc745be9