Closed Bug 1558503 Opened 4 months ago Closed 3 months ago

Order of parameters is lost in code for storageAccess.message

Categories

(Firefox :: Protections UI, defect, P1)

68 Branch
defect

Tracking

()

VERIFIED FIXED
Firefox 70
Tracking Status
firefox-esr60 --- unaffected
firefox-esr68 68+ verified
firefox67 --- wontfix
firefox67.0.1 --- wontfix
firefox68 + verified
firefox69 + verified
firefox70 + verified

People

(Reporter: flod, Assigned: ehsan)

References

(Regression)

Details

(Keywords: regression)

Attachments

(2 files)

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).

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?

Flags: needinfo?(jhofmann)
No longer blocks: 1490811
Has Regression Range: --- → yes
Has STR: --- → yes
Regressed by: 1490811

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!

Flags: needinfo?(jhofmann)
Priority: -- → P1

Ehsan is this something you can take on?

Flags: needinfo?(ehsan)

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

Ehsan is this something you can take on?

Definitely.

Assignee: nobody → ehsan
Flags: needinfo?(ehsan)

(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.

Flags: needinfo?(jhofmann)

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 <>...

Flags: needinfo?(jhofmann)

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.

Flags: needinfo?(ehsan)

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.

Flags: needinfo?(ehsan)

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

(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.

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?

Flags: needinfo?(jcristau)

Seems plausible.

Flags: needinfo?(jcristau) → qe-verify+

(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?

Pushed by eakhgari@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3330dad3168c
Preserve the order of the arguments in the localized storageAccess.message string; r=johannh,flod

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.

(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!

Flags: needinfo?(francesco.lodolo)

(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.

Flags: needinfo?(francesco.lodolo)

(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...

Status: NEW → RESOLVED
Closed: 3 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 70

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

Flags: needinfo?(francesco.lodolo)

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.

Flags: needinfo?(francesco.lodolo)
Attached image 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 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.
  1. Set the dom.storage_access.auto_grants pref to false.
  2. Set the urlclassifier.trackingAnnotationTable.testEntries pref to storage-access.englehardt-tracker.com.
  3. Set the network.cookie.cookieBehavior pref to 4.
  4. Load https://storage-access.englehardt-tracker.com/index.html and click somewhere on the page.
  5. Load https://senglehardt.com/test/storage_access/, and inside the iframe on the left hand side, click on the "Call document.requestStorageAccess()" button.
  6. 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
Attachment #9076906 - Flags: approval-mozilla-release?
Attachment #9076906 - Flags: approval-mozilla-beta?

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!

Flags: needinfo?(francesco.lodolo)
Flags: needinfo?(ehsan)

(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!

Flags: needinfo?(ehsan)

Thank you!

Flags: qe-verify+

Still needs verification on uplift.

Flags: qe-verify+
Flags: needinfo?(francesco.lodolo)

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

Still needs verification on uplift.

It was verified in comment 23.

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

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.

Attachment #9076906 - Flags: approval-mozilla-esr68+
Attachment #9076906 - Flags: approval-mozilla-beta?
Attachment #9076906 - Flags: approval-mozilla-beta+
QA Whiteboard: [qa-triaged]

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 on attachment 9076906 [details]
Bug 1558503 - Preserve the order of the arguments in the localized storageAccess.message string;

i18n fix for 68.0.1

Attachment #9076906 - Flags: approval-mozilla-release? → approval-mozilla-release+

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.

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

Status: RESOLVED → VERIFIED
QA Whiteboard: [qa-triaged]
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.