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

RESOLVED FIXED in Firefox 65

Status

()

P1
normal
RESOLVED FIXED
5 months ago
4 months ago

People

(Reporter: MattN, Assigned: dpino)

Tracking

unspecified
mozilla65
Points:
---
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox65 fixed)

Details

(Whiteboard: [webpayments-reserve])

Attachments

(1 attachment, 10 obsolete attachments)

5.87 KB, patch
baku
: review+
Details | Diff | Splinter Review
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.
Priority: P3 → P2
(Assignee)

Comment 1

5 months ago
Attachment #9022835 - Flags: review?(MattN+bmo)
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.
Attachment #9022835 - Flags: review+
Assignee: nobody → dpino
Status: NEW → ASSIGNED
Priority: P2 → P1
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?
Attachment #9022835 - Flags: review?(amarchesini)
Attachment #9022835 - Flags: review?(MattN+bmo)
Attachment #9022835 - Flags: feedback+
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()
Attachment #9022835 - Flags: review?(amarchesini) → review-
(Assignee)

Comment 6

5 months ago
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
Attachment #9022835 - Attachment is obsolete: true
Attachment #9023212 - Flags: review?(amarchesini)
Attachment #9023212 - Flags: review?(MattN+bmo)
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.
Attachment #9023212 - Flags: review?(amarchesini) → review-
(Assignee)

Comment 8

5 months ago
Addressed comments from latest review (moved callback to PaymentRequestManager).
Attachment #9023212 - Attachment is obsolete: true
Attachment #9023212 - Flags: review?(MattN+bmo)
Attachment #9023260 - Flags: review?(amarchesini)
Attachment #9023260 - Flags: review?(MattN+bmo)
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;
}
Attachment #9023260 - Flags: review?(amarchesini) → review+
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
Attachment #9023260 - Flags: review?(MattN+bmo) → review+
(Assignee)

Comment 11

5 months ago
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.
Attachment #9023260 - Attachment is obsolete: true
Attachment #9023540 - Flags: review?(amarchesini)
Attachment #9023540 - Flags: review?(MattN+bmo)
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...
Attachment #9023540 - Flags: review?(amarchesini) → review+
(Assignee)

Comment 13

5 months ago
Updated patch (IsRegionSupported() and list of regions stored as array).
Attachment #9023540 - Attachment is obsolete: true
Attachment #9023540 - Flags: review?(MattN+bmo)
Attachment #9023546 - Flags: review?(amarchesini)
Attachment #9023546 - Flags: review?(MattN+bmo)
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;
Attachment #9023546 - Flags: review?(amarchesini) → review+
(Assignee)

Comment 15

5 months ago
Amended requested changes from previous review.
Attachment #9023546 - Attachment is obsolete: true
Attachment #9023546 - Flags: review?(MattN+bmo)
Attachment #9023589 - Flags: review?(amarchesini)
Attachment #9023589 - Flags: review?(MattN+bmo)
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)
Attachment #9023589 - Flags: review?(amarchesini) → review+
(Assignee)

Comment 17

5 months ago
Amended change suggestion from last review.
Attachment #9023589 - Attachment is obsolete: true
Attachment #9023589 - Flags: review?(MattN+bmo)
Attachment #9023605 - Flags: review?(amarchesini)
Attachment #9023605 - Flags: review?(MattN+bmo)
Comment on attachment 9023605 [details] [diff] [review]
Bug-1501102-Move-hard-coded-supportedRegions-array-t.patch

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

Thanks
Attachment #9023605 - Flags: review?(MattN+bmo) → review+
Attachment #9023605 - Flags: review?(amarchesini) → review+
(Assignee)

Updated

5 months ago
Keywords: checkin-needed

Comment 20

5 months ago
Pushed by ncsoregi@mozilla.com:
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
Keywords: checkin-needed

Comment 21

5 months ago
Backout by rgurzau@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/520b6ade8bc3
Backed out changeset f1e6ccbef48d for linux leakcheck failures on a CLOSED TREE
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.
Flags: needinfo?(dpino)
(Assignee)

Comment 23

5 months ago
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.
Flags: needinfo?(dpino)
(Assignee)

Updated

5 months ago
Flags: needinfo?(amarchesini)
Flags: needinfo?(MattN+bmo)
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
Flags: needinfo?(MattN+bmo)
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.
(Assignee)

Comment 26

5 months ago
Attachment #9023605 - Attachment is obsolete: true
Flags: needinfo?(amarchesini)
(Assignee)

Comment 28

5 months ago
Attachment #9024022 - Attachment is obsolete: true
(Assignee)

Comment 30

4 months ago
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
(Assignee)

Updated

4 months ago
Flags: needinfo?(amarchesini)
Flags: needinfo?(MattN+bmo)
Baku can help you.
Flags: needinfo?(MattN+bmo)
Ok, move the array into the class. You don't need it static. Right?
Flags: needinfo?(amarchesini)
Flags: qe-verify-
(Assignee)

Comment 33

4 months ago
Made gSupportedRegions a member variable.
Attachment #9024140 - Attachment is obsolete: true
Attachment #9024942 - Flags: review?(amarchesini)
(Assignee)

Comment 35

4 months ago
Amended compilation error in previous patch.
Attachment #9024942 - Attachment is obsolete: true
Attachment #9024942 - Flags: review?(amarchesini)
Attachment #9024952 - Flags: review?(amarchesini)
(Assignee)

Comment 37

4 months ago
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.
Attachment #9024952 - Flags: review?(amarchesini) → review+
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.
Keywords: checkin-needed

Comment 39

4 months ago
Pushed by ccoroiu@mozilla.com:
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
Keywords: checkin-needed

Comment 40

4 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/de5d5a93d95b
Status: ASSIGNED → RESOLVED
Last Resolved: 4 months ago
status-firefox65: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
You need to log in before you can comment on or make changes to this bug.