1558503 - Order of parameters is lost in code for storageAccess.message

Status: VERIFIED FIXED

(Firefox :: Protections UI, defect, P1)

Description

[Francesco Lodolo [:flod]]

Discovered by Pike while reviewing bug 1557793.

storageAccess.message = Will you give %1$S access to track your browsing activity on %2$S?

The string has ordered arguments, that are completely lost when you do this.

  get message() {
    return gBrowserBundle.formatStringFromName("storageAccess.message", ["<>", "<>"], 2);
  },

I can count at least 4 locales broken by this piece of code (Japanese is the most relevant one).

Comment 1

[(no longer active)]

Oops, my apologies. :-(

Johann, what are your thoughts about how to fix this? One possible option is to rewrite _formatDescriptionMessage as a recursive function that breaks its input based on either "<>" or "{}" and have "{}" be reserved for the second name. What do you think?

Comment 2

[Johann Hofmann [:johannh]]

Oh, what an interesting bug. I don't really have a better suggestion than using {} to annotate the secondName placeholder, though I don't fully understand the recursive part. Can't you just check for the presence of {} and either split on it or append the whole remaining string?

Overall it seems like the easiest way forward. :)

Thanks!

Comment 3

[Julien Cristau [:jcristau]]

Ehsan is this something you can take on?

Comment 4

[(no longer active)]

(In reply to Julien Cristau [:jcristau] from comment #3)

Ehsan is this something you can take on?

Definitely.

Comment 5

[(no longer active)]

(In reply to Johann Hofmann [:johannh] (Away until July 3rd) from comment #2)

Oh, what an interesting bug. I don't really have a better suggestion than using {} to annotate the secondName placeholder

OK, no problem. The token we decide to use probably doesn't matter all that much...

though I don't fully understand the recursive part. Can't you just check for the presence of {} and either split on it or append the whole remaining string?

Hmm, still thinking about this... Not sure why I thought the recursiveness of the function is even worth mentioning! But now that I'm thinking about this again, I think the proper way to fix this is to make _formatDescriptionMessage actually process %1$s and %2$s directly without replacing them with <>, {} and the like. I'd like to double check if you're OK with that before spending time to write the code.

Comment 6

[Johann Hofmann [:johannh]]

Yeah, that makes total sense (processing %1$s directly). I'm not sure why we didn't do that before, presumably it was easier to just handle <>...

Comment 7

[(no longer active)]

I looked at this more today, and unfortunately the previous idea that I had wouldn't work, since we have code like this in the tree which effectively collapses one of the original arguments in the message string.

I hope to have enough time to create a patch tomorrow.

Comment 8

[(no longer active)]

I have a fix for this. I tested it by changing storageAccess.message locally to have %2$S appear before %1$S in that message and verifying that the correct domain names appear in the string.

Comment 9

[(no longer active)]

Attachment: Bug 1558503 - Preserve the order of the arguments in the localized storageAccess.message string;

Comment 10

[Francesco Lodolo [:flod]]

At risk of asking a very dumb question: why do we need to replace the original parameters at all? Can't we replace them directly?

That's what this utility does, and it seems like the safest approach (it scales to more than 2, if nothing else)
https://searchfox.org/mozilla-central/rev/40ef22080910c2e2c27d9e2120642376b1d8b8b2/toolkit/modules/BrowserUtils.jsm#705

Comment 11

[(no longer active)]

(In reply to Francesco Lodolo [:flod] from comment #10)

At risk of asking a very dumb question: why do we need to replace the original parameters at all? Can't we replace them directly?

That's what this utility does, and it seems like the safest approach (it scales to more than 2, if nothing else)
https://searchfox.org/mozilla-central/rev/40ef22080910c2e2c27d9e2120642376b1d8b8b2/toolkit/modules/BrowserUtils.jsm#705

This is similar to what I was suggesting to do around comment 5 (except that I didn't know about this helpful utility function.) The problem is that this code is used in many existing places for other notifications, and some of those notifications use strings with more than one placeholder where the notification code is expected to see only one of them (see comment 7).

For one example, see https://searchfox.org/mozilla-central/rev/40ef22080910c2e2c27d9e2120642376b1d8b8b2/browser/locales/en-US/chrome/browser/browser.properties#461. Here, %$2S will be replaced with brandShortName (see https://searchfox.org/mozilla-central/rev/da3f3eaaacb6fb344fd21ac29ace2da0e33f12d3/browser/base/content/browser.js#8477-8481) and %$1S will be replaced with <> which the notification code will see and it will replace with the site's origin.

I could do a more sophisticated kind of surgery where all such arguments would be dealt with inside the notification code and add the necessary smarts to the notification code to teach it what to replace these placeholders with, but I'd also like to backport this patch to beta and release, so I would really like to keep the changes as small and non-invasive as I can, which is why I went with this approach. I was also very worried that if I chose the more invasive approach, there would be some notification call site that I fail to find while grepping for "<>" and cause regressions because of that.

Hope that explains why I went with the approach in the attached patch.

Comment 12

[(no longer active)]

Julien, I'd like to take this patch as a possible 68 dot release ride-along as well. This is a fix for four of our localizations, and should be rather low risk. Do you think that is a possibility?

Comment 13

[Julien Cristau [:jcristau]]

Seems plausible.

Comment 14

[Francesco Lodolo [:flod]]

(In reply to :Ehsan Akhgari from comment #11)

I could do a more sophisticated kind of surgery where all such arguments would be dealt with inside the notification code and add the necessary smarts to the notification code to teach it what to replace these placeholders with, but I'd also like to backport this patch to beta and release, so I would really like to keep the changes as small and non-invasive as I can, which is why I went with this approach. I was also very worried that if I chose the more invasive approach, there would be some notification call site that I fail to find while grepping for "<>" and cause regressions because of that.

OK, that makes sense. Can we file a bug and address it as a follow-up at some point?

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3330dad3168c
Preserve the order of the arguments in the localized storageAccess.message string; r=johannh,flod

Comment 16

[Axel Hecht [:Pike]]

This sounds like one of those things that we should implement very differently in Fluent.

There's precedent in for example the notificationbox taking data-l10n-ids as messages, this might be something we can do when converting to Fluent, too.

Comment 17

[(no longer active)]

(In reply to Francesco Lodolo [:flod] from comment #14)

(In reply to :Ehsan Akhgari from comment #11)

I could do a more sophisticated kind of surgery where all such arguments would be dealt with inside the notification code and add the necessary smarts to the notification code to teach it what to replace these placeholders with, but I'd also like to backport this patch to beta and release, so I would really like to keep the changes as small and non-invasive as I can, which is why I went with this approach. I was also very worried that if I chose the more invasive approach, there would be some notification call site that I fail to find while grepping for "<>" and cause regressions because of that.

OK, that makes sense. Can we file a bug and address it as a follow-up at some point?

I'd be happy to, but I'm not sure what you're suggesting specifically to do. Can you please be more explicit about it (since I can see a few possibilities here). Thanks!

(In reply to Axel Hecht [:Pike] from comment #16)

This sounds like one of those things that we should implement very differently in Fluent.

There's precedent in for example the notificationbox taking data-l10n-ids as messages, this might be something we can do when converting to Fluent, too.

Very likely. I'm unfortunately not as well versed in Fluent as I'd liked to be, so I have a hard time imagining good alternatives. Any help is appreciated!

Comment 18

[Francesco Lodolo [:flod]]

(In reply to :Ehsan Akhgari from comment #17)

I'd be happy to, but I'm not sure what you're suggesting specifically to do. Can you please be more explicit about it (since I can see a few possibilities here). Thanks!

Remove code that relies on intermediate interpolations of the strings, and use the strings directly.

There's only one possible defined format supported for properties placeholders (%S or %n$S), turns out we have code that replaces these placeables, to then replace them a second time to inject HTML markup. The code should just should inject this markup in the original string.

I don't see how the current approach can scale. If we ever need a third placeable, we'll need another combination of characters to support it.

Comment 19

[(no longer active)]

(In reply to Francesco Lodolo [:flod] from comment #18)

(In reply to :Ehsan Akhgari from comment #17)

I'd be happy to, but I'm not sure what you're suggesting specifically to do. Can you please be more explicit about it (since I can see a few possibilities here). Thanks!

Remove code that relies on intermediate interpolations of the strings, and use the strings directly.

I filed bug 1564986. Apologies if that's not what you wanted, I still don't understand all of the pieces of what a good solution would look like here, but we can move the discussion there. Feel free to edit things as needed. :-)

There's only one possible defined format supported for properties placeholders (%S or %n$S), turns out we have code that replaces these placeables, to then replace them a second time to inject HTML markup. The code should just should inject this markup in the original string.

Oh, so one thing that maybe I should have explained here is that nobody is generating HTML markup here. _formatDescriptionMessage just prepares the pieces of the string that will later on be put inside this existing markup using these rules.

I don't see how the current approach can scale. If we ever need a third placeable, we'll need another combination of characters to support it.

Of course, I'm not really defending the current approach. To be clear, we just built a hack on top of an existing hack without knowing that it would have caused a problem for localizers...

Comment 20

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/3330dad3168c

Comment 21

[(no longer active)]

Is there anything specific we need to do to verify this bug (beyond the local testing that I did)? Thanks!

Comment 22

[Francesco Lodolo [:flod]]

The only thing I can think of, is to try a Japanese nightly, if you can trigger that dialog easily, and verify that the order of parameters is respected.

Comment 23

[(no longer active)]

Attachment: Screenshot

Good idea!

Here is a screenshot, based on the link in comment 0, we would expect the name of the top-level origin (senglehardt.com) to appear first (since %$2S appears first in the Japanese string), so the bug looks fixed.

Comment 24

[(no longer active)]

Comment on attachment 9076906 [details]
Bug 1558503 - Preserve the order of the arguments in the localized storageAccess.message string;

Beta/Release Uplift Approval Request

• User impact if declined: Users of four of our locales will see the wrong message in our Storage Access API doorhangers. The order of the two domain names in that doorhanger would be flipped (see attachment 9077399 [details] for how the correct doorhanger must look like in a Japanese build.)
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: 1. Download a Japanese build from the branch we'd like to test and run it.

2. Set the dom.storage_access.auto_grants pref to false.
3. Set the urlclassifier.trackingAnnotationTable.testEntries pref to storage-access.englehardt-tracker.com.
4. Set the network.cookie.cookieBehavior pref to 4.
5. Load https://storage-access.englehardt-tracker.com/index.html and click somewhere on the page.
6. Load https://senglehardt.com/test/storage_access/, and inside the iframe on the left hand side, click on the "Call document.requestStorageAccess()" button.
7. Compare the doorhanger that opens to attachment 9077399 [details]. Make sure the first domain listed is "senglehardt.com" and the second one listed is "storage-access.senglehardt-tracker.com".

• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): The patch is fairly low risk, it only changes the Storage Access API doorhanger without making changes to other notification doorhangers. On other locales besides the four affected it should be a no-op.
• String changes made/needed: None

Comment 25

[Daniel Bodea [:danibodea]]

I can also spend time to verify this fix, but I would need detailed information on how to reproduce it in the first place. Do you think it's OK for me to get involved? If yes, please provide the necessary information. Thanks!

Comment 26

[(no longer active)]

(In reply to Bodea Daniel [:danibodea] from comment #25)

I can also spend time to verify this fix, but I would need detailed information on how to reproduce it in the first place. Do you think it's OK for me to get involved? If yes, please provide the necessary information. Thanks!

The STR to verify this is in comment 24, but I already verified it on trunk, I don't think it makes sense to do that again. :-) We'd need to verify it on branches when it lands there. Thanks!

Comment 27

[Daniel Bodea [:danibodea]]

Thank you!

Comment 28

[Julien Cristau [:jcristau]]

Still needs verification on uplift.

Comment 29

[(no longer active)]

(In reply to Julien Cristau [:jcristau] from comment #28)

Still needs verification on uplift.

It was verified in comment 23.

Comment 30

[Julien Cristau [:jcristau]]

I meant we should do the same thing after uplift, which is why I put the flag back. :)

Comment 31

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9076906 [details]
Bug 1558503 - Preserve the order of the arguments in the localized storageAccess.message string;

Fixes incorrect Storage Access API doorhanger messages for some locales. Approved for 69.0b4. We should also take this for 68.1esr. Deferring to Julien for the 68.0.x decision as the 68 release owner.

Comment 32

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/3bcc11455906

Comment 33

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr68/rev/a1d610b7cb6f

Comment 34

[Daniel Bodea [:danibodea]]

I have verified this fix in Beta v69.0b4 on Windows 10 using the steps in the uplift approval request.
A JP build with the fix pushed in ESR 68 could not be found, yet.

Leaving "qe-verify+" tag since it also needs to be verified in est68. Thanks.

Comment 35

[Julien Cristau [:jcristau]]

Comment on attachment 9076906 [details]
Bug 1558503 - Preserve the order of the arguments in the localized storageAccess.message string;

i18n fix for 68.0.1

Comment 36

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-release/rev/35ad10ab4145

Comment 37

[Ryan VanderMeulen [:RyanVM]]

Per discussion with jcristau, we're uplifting this to 68.0.1esr also to maintain parity with the non-ESR 68.0.1 release and hopefully avoid some confusion.

Comment 38

[Ryan VanderMeulen [:RyanVM]]

FIREFOX_ESR_68_0_X_RELBRANCH (68.0.1esr): https://hg.mozilla.org/releases/mozilla-esr68/rev/a244194949587eaa74bcf9f9015388214e36a2d7

Comment 39

[Rares Doghi, Desktop QA]

Hi, this issue is Verified as Fixed in 68.0.1esr as well as Firefox 68.0.1, I Will mark this issue accordingly.