1501102 - Move hard-coded `supportedRegions` array to a pref to allow developers outside US/CA to test PaymentRequest

Status: RESOLVED FIXED

(Core :: DOM: Web Payments, enhancement, P1)

Description

[Matthew N. [:MattN]]

Similar to `extensions.formautofill.supportedCountries` which takes a comma-separated list of countries, we should have a new pref: `dom.payments.request.supportedCountries` with the value "US,CA" to allow developers outside US/CA to test PaymentRequest.

Comment 1

[Diego Pino]

Attachment: Bug-1501102-Move-hard-coded-supportedRegions-array-t.patch

Comment 2

[Diego Pino]

Try https://treeherder.mozilla.org/#/jobs?repo=try&revision=db24ec94da2dc1fb7693155455625d200a6dcdf5

Comment 3

[Marcos Caceres [:marcosc]]

Comment on attachment 9022835 [details] [diff] [review]
Bug-1501102-Move-hard-coded-supportedRegions-array-t.patch

Review of attachment 9022835 [details] [diff] [review]:
-----------------------------------------------------------------

lgtm - this is a great addition.

Comment 4

[Matthew N. [:MattN]]

Comment on attachment 9022835 [details] [diff] [review]
Bug-1501102-Move-hard-coded-supportedRegions-array-t.patch

Review of attachment 9022835 [details] [diff] [review]:
-----------------------------------------------------------------

I think this needs a DOM peer review so redirecting to baku

::: browser/app/profile/firefox.js
@@ +1706,5 @@
>  
> +#ifdef NIGHTLY_BUILD
> +// List of regions where PaymentRequest is available by default.
> +pref("dom.payments.request.supportedRegions", "US,CA");
> +#endif

If we need this in a JS file, could you put it with the other payment prefs please? https://dxr.mozilla.org/mozilla-central/rev/c291143e24019097d087f9307e59b49facaf90cb/modules/libpref/init/all.js#5835

::: dom/payments/PaymentRequest.cpp
@@ +65,5 @@
>  PaymentRequest::PrefEnabled(JSContext* aCx, JSObject* aObj)
>  {
>  #if defined(NIGHTLY_BUILD)
> +  nsAutoString supportedRegions;
> +  Preferences::GetString("dom.payments.request.supportedRegions", supportedRegions);

Shouldn't this also use StaticPrefs?

Comment 5

[Andrea Marchesini [:baku]]

Comment on attachment 9022835 [details] [diff] [review]
Bug-1501102-Move-hard-coded-supportedRegions-array-t.patch

Review of attachment 9022835 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/payments/PaymentRequest.cpp
@@ +65,5 @@
>  PaymentRequest::PrefEnabled(JSContext* aCx, JSObject* aObj)
>  {
>  #if defined(NIGHTLY_BUILD)
> +  nsAutoString supportedRegions;
> +  Preferences::GetString("dom.payments.request.supportedRegions", supportedRegions);

This pref must be cached. See Preferences::RegisterCallbackAndCall()

Comment 6

[Diego Pino]

Attachment: Bug-1501102-Move-hard-coded-supportedRegions-array-t.patch

Addressed comments in the latest review.

I'm not sure about what's the best way to store the preference value from the callback. I stored the value as a member variable, but I've seen in other code that values stored from a callback are store in a different manner (using Atomic), for instance: https://searchfox.org/mozilla-central/source/dom/quota/QuotaManagerService.cpp#52

Comment 7

[Andrea Marchesini [:baku]]

Comment on attachment 9023212 [details] [diff] [review]
Bug-1501102-Move-hard-coded-supportedRegions-array-t.patch

Review of attachment 9023212 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/payments/PaymentRequest.cpp
@@ +77,5 @@
>  bool
>  PaymentRequest::PrefEnabled(JSContext* aCx, JSObject* aObj)
>  {
>  #if defined(NIGHTLY_BUILD)
> +  Preferences::RegisterCallbackAndCall(SupportedRegionsPrefChangedCallback,

This is the wrong place. PaymentRequest can be created and destroyed several times.
This code should go to PaymentRequestManager.

Comment 8

[Diego Pino]

Attachment: Bug-1501102-Move-hard-coded-supportedRegions-array-t.patch

Addressed comments from latest review (moved callback to PaymentRequestManager).

Comment 9

[Andrea Marchesini [:baku]]

Comment on attachment 9023260 [details] [diff] [review]
Bug-1501102-Move-hard-coded-supportedRegions-array-t.patch

Review of attachment 9023260 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks

::: dom/payments/PaymentRequest.cpp
@@ +66,5 @@
>  {
>  #if defined(NIGHTLY_BUILD)
> +  RefPtr<PaymentRequestManager> manager = PaymentRequestManager::GetSingleton();
> +  MOZ_ASSERT(manager);
> +  nsString supportedRegions = manager->GetSupportedRegions();

nsAutoString supportedRegions;
manager->GetSupportedRegions(supportedRegions);

::: dom/payments/PaymentRequestManager.cpp
@@ +322,5 @@
> +                                  kSupportedRegionsPref);
> +}
> +
> +nsString
> +PaymentRequestManager::GetSupportedRegions()

what about:

void
PaymentRequestManager::GetSupportedRegions(nsAstring& aSupportedRegions)
{
  aSupportedRegions = gSupportedRegions;
}

Comment 10

[Matthew N. [:MattN]]

Comment on attachment 9023260 [details] [diff] [review]
Bug-1501102-Move-hard-coded-supportedRegions-array-t.patch

Review of attachment 9023260 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/payments/PaymentRequestManager.cpp
@@ +305,5 @@
> +  MOZ_ASSERT(NS_IsMainThread());
> +  MOZ_ASSERT(!strcmp(aPrefName, kSupportedRegionsPref));
> +  MOZ_ASSERT(!aClosure);
> +
> +  Preferences::GetString(aPrefName, gSupportedRegions);

Wouldn't it make more sense to do the splitting on commas here so that it would be cached rather than splitting each time in PaymentRequest? Not sure if baku agrees

Comment 11

[Diego Pino]

Attachment: Bug-1501102-Move-hard-coded-supportedRegions-array-t.patch

Addressed comments from latest review.

I have another patch that stores the list of supported regions as an array, as :mattn suggested. Let me know if that's your preference and I'll update the patch.

Comment 12

[Andrea Marchesini [:baku]]

Comment on attachment 9023540 [details] [diff] [review]
Bug-1501102-Move-hard-coded-supportedRegions-array-t.patch

Review of attachment 9023540 [details] [diff] [review]:
-----------------------------------------------------------------

Here a different approach. Up to you.

::: dom/payments/PaymentRequest.cpp
@@ +67,5 @@
>  #if defined(NIGHTLY_BUILD)
> +  RefPtr<PaymentRequestManager> manager = PaymentRequestManager::GetSingleton();
> +  MOZ_ASSERT(manager);
> +  nsAutoString supportedRegions;
> +  manager->GetSupportedRegions(supportedRegions);

Actually, yes, I prefer to have:

if (!manager->IsRegionSupported(region)) {
  return false;
}

::: dom/payments/PaymentRequestManager.cpp
@@ +296,5 @@
>  /* PaymentRequestManager */
>  
>  StaticRefPtr<PaymentRequestManager> gPaymentManager;
> +const char kSupportedRegionsPref[] = "dom.payments.request.supportedRegions";
> +nsString gSupportedRegions;

nsTArray<nsString> gSupportedRegions;

@@ +305,5 @@
> +  MOZ_ASSERT(NS_IsMainThread());
> +  MOZ_ASSERT(!strcmp(aPrefName, kSupportedRegionsPref));
> +  MOZ_ASSERT(!aClosure);
> +
> +  Preferences::GetString(aPrefName, gSupportedRegions);

nsAutoString supportedRegions;
Preferences::GetStrin(aPrefName, supportedRegions);
here the split...

Comment 13

[Diego Pino]

Attachment: Bug-1501102-Move-hard-coded-supportedRegions-array-t.patch

Updated patch (IsRegionSupported() and list of regions stored as array).

Comment 14

[Andrea Marchesini [:baku]]

Comment on attachment 9023546 [details] [diff] [review]
Bug-1501102-Move-hard-coded-supportedRegions-array-t.patch

Review of attachment 9023546 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/payments/PaymentRequestManager.cpp
@@ +304,5 @@
> +{
> +  MOZ_ASSERT(NS_IsMainThread());
> +  MOZ_ASSERT(!strcmp(aPrefName, kSupportedRegionsPref));
> +  MOZ_ASSERT(!aClosure);
> +

gSupportedRegions.Clear();

otherwise, you don't remove the previous regions when the pref changes.

@@ +326,5 @@
> +                                  kSupportedRegionsPref);
> +}
> +
> +bool
> +PaymentRequestManager::IsRegionSupported(nsAutoString region)

1. const nsAString& aRegion
2. return gSupportedRegions.Contains(aRegion);

::: dom/payments/PaymentRequestManager.h
@@ +73,5 @@
>                               const nsAString& aPayerName,
>                               const nsAString& aPayerEmail,
>                               const nsAString& aPayerPhone);
>  
> +  bool IsRegionSupported(nsAutoString region);

bool IsRegionSupported(const nsAString& aRegion) const;

Comment 15

[Diego Pino]

Attachment: Bug-1501102-Move-hard-coded-supportedRegions-array-t.patch

Amended requested changes from previous review.

Comment 16

[Andrea Marchesini [:baku]]

Comment on attachment 9023589 [details] [diff] [review]
Bug-1501102-Move-hard-coded-supportedRegions-array-t.patch

Review of attachment 9023589 [details] [diff] [review]:
-----------------------------------------------------------------

perfect. thanks.

::: dom/payments/PaymentRequestManager.cpp
@@ +309,5 @@
> +  nsAutoString supportedRegions;
> +  Preferences::GetString(aPrefName, supportedRegions);
> +  gSupportedRegions.Clear();
> +  for (const nsAString& each : supportedRegions.Split(',')) {
> +    gSupportedRegions.InsertElementAt(0, each);

gSupportedRegions.AppendElement(each)

Comment 17

[Diego Pino]

Attachment: Bug-1501102-Move-hard-coded-supportedRegions-array-t.patch

Amended change suggestion from last review.

Comment 18

[Matthew N. [:MattN]]

Comment on attachment 9023605 [details] [diff] [review]
Bug-1501102-Move-hard-coded-supportedRegions-array-t.patch

Review of attachment 9023605 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks

Comment 19

[Diego Pino]

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ba1c1c82d622e822d7c204f7e16eeea279bae369

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f1e6ccbef48d
Move hard-coded 'supportedRegions' array to a pref to allow developers outside US/CA to test PaymentRequest. r=baku

Comment 21

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/520b6ade8bc3
Backed out changeset f1e6ccbef48d for linux leakcheck failures on a CLOSED TREE

Comment 22

[Raul Gurzau (:RaulG)]

Backed out changeset f1e6ccbef48d (bug 1501102) for linux leakcheck failures on a CLOSED TREE 

Backout link: https://hg.mozilla.org/integration/mozilla-inbound/rev/520b6ade8bc3994e98b1b2f82274c9b9f2b46db2

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&resultStatus=testfailed%2Cbusted%2Cexception&selectedJob=210673669&revision=f1e6ccbef48d843bff32d5b1c6d222041de98436

Log link: https://treeherder.mozilla.org/logviewer.html#?job_id=210673715&repo=mozilla-inbound&lineNumber=1832

Log snippet: 

[task 2018-11-08T22:50:06.455Z] 22:50:06     INFO - TEST-INFO | Main app process: exit 0
[task 2018-11-08T22:50:06.456Z] 22:50:06     INFO - runtests.py | Application ran for: 0:00:17.673165
[task 2018-11-08T22:50:06.457Z] 22:50:06     INFO - zombiecheck | Reading PID log: /tmp/tmpGADrV9pidlog
[task 2018-11-08T22:50:06.458Z] 22:50:06     INFO - ==> process 1057 launched child process 1078
[task 2018-11-08T22:50:06.458Z] 22:50:06     INFO - zombiecheck | Checking for orphan process with PID: 1078
[task 2018-11-08T22:50:06.458Z] 22:50:06     INFO - Stopping web server
[task 2018-11-08T22:50:06.479Z] 22:50:06     INFO - Stopping web socket server
[task 2018-11-08T22:50:06.500Z] 22:50:06     INFO - Stopping ssltunnel
[task 2018-11-08T22:50:06.522Z] 22:50:06     INFO - TEST-INFO | leakcheck | default process: leak threshold set at 0 bytes
[task 2018-11-08T22:50:06.522Z] 22:50:06     INFO - TEST-INFO | leakcheck | plugin process: leak threshold set at 0 bytes
[task 2018-11-08T22:50:06.524Z] 22:50:06     INFO - TEST-INFO | leakcheck | tab process: leak threshold set at 0 bytes
[task 2018-11-08T22:50:06.524Z] 22:50:06     INFO - TEST-INFO | leakcheck | geckomediaplugin process: leak threshold set at 20000 bytes
[task 2018-11-08T22:50:06.524Z] 22:50:06     INFO - TEST-INFO | leakcheck | gpu process: leak threshold set at 0 bytes
[task 2018-11-08T22:50:06.526Z] 22:50:06     INFO - 
[task 2018-11-08T22:50:06.527Z] 22:50:06     INFO - == BloatView: ALL (cumulative) LEAK AND BLOAT STATISTICS, default process 1057
[task 2018-11-08T22:50:06.528Z] 22:50:06     INFO - 
[task 2018-11-08T22:50:06.529Z] 22:50:06     INFO -      |<----------------Class--------------->|<-----Bytes------>|<----Objects---->|
[task 2018-11-08T22:50:06.531Z] 22:50:06     INFO -      |                                      | Per-Inst   Leaked|   Total      Rem|
[task 2018-11-08T22:50:06.532Z] 22:50:06     INFO -    0 |TOTAL                                 |       27        4| 3977199        1|
[task 2018-11-08T22:50:06.540Z] 22:50:06     INFO - 1595 |nsTArray_base                         |        4        4| 1797722        1|
[task 2018-11-08T22:50:06.542Z] 22:50:06     INFO - 
[task 2018-11-08T22:50:06.542Z] 22:50:06     INFO - nsTraceRefcnt::DumpStatistics: 1733 entries
[task 2018-11-08T22:50:06.543Z] 22:50:06     INFO - TEST-INFO | leakcheck | default process: leaked 1 nsTArray_base
[task 2018-11-08T22:50:06.544Z] 22:50:06    ERROR - TEST-UNEXPECTED-FAIL | leakcheck | default process: 4 bytes leaked (nsTArray_base)
[task 2018-11-08T22:50:06.544Z] 22:50:06     INFO - runtests.py | Running tests: end.

Comment 23

[Diego Pino]

I tried to reproduce the issue locally by inspecting the logs but I haven't managed to figure out what test to run to reproduce the issue. I tried running "./mach test dom/payments/" and "./mach test browser/components/payments/test/browser/" but both runs ended without fails.

I believe the leak error is likely related to "nsTArray<nsString> gSupportedRegions". Would it be possible that the copy of split region elements need move semantics (so gSupportedRegions is the owner and its elements get removed when out object is destroyed). 


```
for (const nsAString& each : supportedRegions.Split(',')) {
    gSupportedRegions.AppendElement(std::move(each));
  }
}
```

I don't know, I'm just speculating here. The leak might be somewhere else.

Comment 24

[Matthew N. [:MattN]]

Baku, can help you with a solution as I'm not a C++ developer. It very likely is due to the gSupportedRegions array though. You should be able to reproduce it locally if your mozconfig file has `ac_add_options --enable-debug`

https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Configuring_Build_Options#Selecting_Build_Options

Comment 25

[Andrea Marchesini [:baku]]

Comment on attachment 9023605 [details] [diff] [review]
Bug-1501102-Move-hard-coded-supportedRegions-array-t.patch

Review of attachment 9023605 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/payments/PaymentRequestManager.cpp
@@ +323,5 @@
> +PaymentRequestManager::~PaymentRequestManager()
> +{
> +  MOZ_ASSERT(mActivePayments.Count() == 0);
> +  Preferences::UnregisterCallback(SupportedRegionsPrefChangedCallback,
> +                                  kSupportedRegionsPref);

Cleanup the array here. This should fix the leak.

Comment 26

[Diego Pino]

Attachment: Bug-1501102-Move-hard-coded-supportedRegions-array-t.patch

Comment 27

[Diego Pino]

Updated patch with suggested fix and run try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=bcf3acd5dbd4a79769f8b96897fcb8e3dc3573dd

Comment 28

[Diego Pino]

Attachment: Bug-1501102-Move-hard-coded-supportedRegions-array-t.patch

Comment 29

[Diego Pino]

Try https://treeherder.mozilla.org/#/jobs?repo=try&revision=23e9bfb8b67646f68025bb11ad2606984b653049

Comment 30

[Diego Pino]

I updated the patch using dynamic allocated memory for gSupportedRegions. The patch seems to work fine according to https://bugzilla.mozilla.org/show_bug.cgi?id=1501102#c29. Previously, I pushed a patch where I tried to fix the leak by calling `gSupportedRegions.Clean()`, but unfortunately that didn't seem to fix the issue according to https://treeherder.mozilla.org/#/jobs?repo=try&revision=bcf3acd5dbd4a79769f8b96897fcb8e3dc3573dd

Comment 31

[Matthew N. [:MattN]]

Baku can help you.

Comment 32

[Andrea Marchesini [:baku]]

Ok, move the array into the class. You don't need it static. Right?

Comment 33

[Diego Pino]

Attachment: Bug-1501102-Move-hard-coded-supportedRegions-array-t.patch

Made gSupportedRegions a member variable.

Comment 34

[Diego Pino]

Try https://treeherder.mozilla.org/#/jobs?repo=try&revision=6d283929d8df9b56328d51c8f56b6ee238583430

Comment 35

[Diego Pino]

Attachment: Bug-1501102-Move-hard-coded-supportedRegions-array-t.patch

Amended compilation error in previous patch.

Comment 36

[Diego Pino]

Try https://treeherder.mozilla.org/#/jobs?repo=try&revision=0906b5aa8f69a886cfdfddc30126c61f2343b547

Comment 37

[Diego Pino]

Try build in debug mode looks good (no memory leaks).

:baku Thanks for the multiple back and forth reviews. Perhaps there's something to amend in the latest patch. I'll wait for your review before landing it.

Comment 38

[Matthew N. [:MattN]]

FYI to Hani and Timea that after this change you can change `dom.payments.request.supportedRegions` instead of `browser.search.region` for testing since users really shouldn't ever change the latter.

Comment 39

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/de5d5a93d95b
Move hard-coded 'supportedRegions' array to a pref to allow developers outside US/CA to test PaymentRequest. r=baku,MattN

Comment 40

[Razvan Maries]

https://hg.mozilla.org/mozilla-central/rev/de5d5a93d95b

Comment 41

[Marcos Caceres [:marcosc]]

Sent a PR to caniuse also: https://github.com/Fyrd/caniuse/pull/4621