Closed Bug 1097928 Opened 5 years ago Closed 5 years ago

Convert MozPaymentProvider API to WebIDL

Categories

(Core :: DOM: Core & HTML, defect, P1, major)

x86
Gonk (Firefox OS)
defect

Tracking

()

RESOLVED FIXED
mozilla38
Tracking Status
firefox36 --- wontfix
firefox37 --- affected
firefox38 --- fixed
b2g-v2.2 --- fixed
b2g-master --- fixed

People

(Reporter: krupa.mozbugs, Assigned: ferjm)

References

(Blocks 1 open bug)

Details

(Keywords: regression)

Attachments

(11 files, 17 obsolete files)

464.87 KB, text/plain
Details
2.36 KB, patch
bholley
: review+
Details | Diff | Splinter Review
7.25 KB, patch
bholley
: review+
Details | Diff | Splinter Review
2.70 KB, patch
fabrice
: review+
Details | Diff | Splinter Review
4.96 KB, patch
bholley
: review+
Details | Diff | Splinter Review
46 bytes, text/x-github-pull-request
fabrice
: review+
Details | Review
31.47 KB, patch
fabrice
: review+
Details | Diff | Splinter Review
12.95 KB, patch
fabrice
: review+
Details | Diff | Splinter Review
11.33 KB, patch
fabrice
: review+
Details | Diff | Splinter Review
20.03 KB, patch
wesj
: review+
Details | Diff | Splinter Review
46 bytes, text/x-github-pull-request
Details | Review
Connectivity: wifi
SIM used:
Gaia/device: flame/ central (2.2.0.0-prerelease)
Build number: eng.cltbld.2014112.073256
Platform version: 36.0a1
Build identifier: 2014112040208Git commit info: 2014-11-11 18:27:28 5ae28ff1

steps to reproduce:
1. Launch marketplace-dev on your flame
2. Sign in 
3. Search for :paid
4. Start the purchase of a paid app


expected behavior:
Enter PIN loads

actual behavior:
trusted UI loads and is stuck at the "Loading" screen due to [JavaScript Error: "Error: Exposing privileged accessor properties is prohibited" {file: "https://marketplace-dev.mozflare.net/mozpay/spa/js/main.min.js?bust=6777619d400789e8143c8a6258d90eb404e35f06" line: 64}]

This is currently blocking our testsuite on fxos to be turned on.

NOTE: purchase works fine on 2.1
Blocks: 1013455
Sounds like the cause might be bug 1084245
Depends on: 1084245
The file in question does not seem to reference mozTCPSocket, so you probably want to look at the root cause of that bug, bug 1082450, instead.  I'm not going to play with the dependencies right now to avoid the bug spam cascade.  Ideally you want to identify the WebAPI that's having troubles and file a new bug/find an existing bug for that and depend on that.  Maybe with a non-minified JS file if possible!
I'll take a look and see if I can find what part of our code is triggering this early next week.
Assignee: nobody → scolville
Using latest central build on flame:

Connectivity: wifi
Gaia/device: flame/ central (2.2.0.0-prerelease)
Build number: eng.cltbld.20141118.073038
Platform version: 36.0a1
Build identifier: 20141118040205
Git commit info: 2014-11-18 09:48:15 4aee2569

I can't get this far I get:

I/Marketplace( 2086): Content JS LOG: [model] Found app with lookup key compound-interest
I/Marketplace( 2086):     at u/< (https://marketplace-dev-cdn.allizom.org/media/fireplace/js/include.js?b=1416336048991:4:25764)
I/Marketplace( 2086): Content JS LOG: [buttons] Install requested for Compound interest
I/Marketplace( 2086):     at u/< (https://marketplace-dev-cdn.allizom.org/media/fireplace/js/include.js?b=1416336048991:4:25764)
I/Marketplace( 2086): Content JS LOG: [buttons] Install suspended; user needs to log in
I/Marketplace( 2086):     at u/< (https://marketplace-dev-cdn.allizom.org/media/fireplace/js/include.js?b=1416336048991:4:25764)
I/Marketplace( 2086): Content JS LOG: [login] Allowing unverified emails
I/Marketplace( 2086):     at u/< (https://marketplace-dev-cdn.allizom.org/media/fireplace/js/include.js?b=1416336048991:4:25764)
I/Marketplace( 2086): Content JS LOG: [yulelog] Handled post message from https://marketplace-dev.allizom.org: {"type":"fxa-request"}
I/Marketplace( 2086):     at log (app://packaged.marketplace-dev.allizom.org/main.js:5:8)

and nothing happens no login nothing.

I'll try again and see if I can install the same build you have.
I get the same as comment4 with the original build noted in the STR.
Raised a bug for the other issue as per comment 4: bug 1101132
Priority: -- → P2
I am not able to make a purchase on my Android 4.2.1 device in FF 36 on MP-stage. After pressing the price button, the Web Pay tab is opened and the payment process remains stuck with the "Loading..." throbber.
I have attached the logcat for this issue.
(In reply to Iulian Timis from comment #7)
> Created attachment 8525419 [details]
> android_payment_fail.txt
> 
> I am not able to make a purchase on my Android 4.2.1 device in FF 36 on
> MP-stage. After pressing the price button, the Web Pay tab is opened and the
> payment process remains stuck with the "Loading..." throbber.
> I have attached the logcat for this issue.

Yep that's the same issue as this bug. Thanks for the heads up - that might help us to get to the root cause quicker.
The bit we're blowing up on looks to be mcc/mnc access:

[JavaScript Warning: "Security wrapper denied access to property mcc on privileged Javascript object. Support for exposing privileged objects to untrusted content via __exposedProps__ is being gradually removed - use WebIDL bindings or Components.utils.cloneInto instead. Note that only the first denied property access from a given global object will be reported." {file: "https://marketplace-dev.mozflare.net/mozpay/spa/js/main.min.js?bust=ddcef7bd28382c120a9b6044b6835d484175bb4b" line: 64}]
W/GeckoConsole(  206): [JavaScript Error: "Error: Exposing privileged accessor properties is prohibited" {file: "https://marketplace-dev.mozflare.net/mozpay/spa/js/main.min.js?bust=ddcef7bd28382c120a9b6044b6835d484175bb4b" line: 64}]
Using a proxy to serve un-minned code in place of the minified src - the line the error points to is:

https://github.com/mozilla/spartacus/blob/96eea677bfffdf859fba194479755b22ee0d2bde/public/js/models/transaction.js#L93
blocking-b2g: --- → 2.2?
bholley, would this be related to bug 1084245 (and a few other bugs around that)?
Flags: needinfo?(bobbyholley)
Yes, this sounds like the API is running afoul of the new security restrictions. The best solution is to convert the payment API to WebIDL, though you may need to do something hackier in the interim. Let me know if there's any lack of clarity around what to do there.

Also, please add tests for this to CI.
Flags: needinfo?(bobbyholley)
Assignee: scolville → nobody
(In reply to Bobby Holley (:bholley) from comment #12)
> Yes, this sounds like the API is running afoul of the new security
> restrictions. The best solution is to convert the payment API to WebIDL,
> though you may need to do something hackier in the interim. Let me know if
> there's any lack of clarity around what to do there.

We are talking about the API as documented here:

https://wiki.mozilla.org/WebAPI/WebPaymentProvider

We are consuming this in the Trusted UI in the payment flow. I think the mozPaymentProvider API is the area that needs to be fixed, not our code that consumes it.

Was there a warning or deprecation plan for this change? It seems quite a sudden change for us that has put some Q4 goals in jeopardy.

> Also, please add tests for this to CI.

Not sure which CI that is, I think we need some platform help here.
Flags: needinfo?(ferjmoreno)
Priority: P2 → P1
(In reply to Andy McKay [:andym] from comment #13)
> We are consuming this in the Trusted UI in the payment flow. I think the
> mozPaymentProvider API is the area that needs to be fixed, not our code that
> consumes it.

Oh definitely - sorry if that was unclear. I was assuming that I was addressing the implementors of the mozPay API, not the consumers.
 
> Was there a warning or deprecation plan for this change? It seems quite a
> sudden change for us that has put some Q4 goals in jeopardy.

There isn't supposed to be any visible behavior change.

> > Also, please add tests for this to CI.
> 
> Not sure which CI that is, I think we need some platform help here.

In Gecko ideally, but the Marketplace-side couldn't hurt to add as well. These changes that apparently broke mozPay landed months ago, so it seems pretty bad that we're only noticing now.

Here's what needs to happen:
(1) We need a test in Gecko that detects this failure.
(2) We need to fix bug 1027734, which will make the failure go away.
(3) If (2) isn't backportable, we need to implement a stop-gap solution for the branch. If you point me to the problematic code, I can suggest a solution.
Bobby and I have been talking about this via IRC. As I was saying to him, I don´t think bug 1027734 is going to help here. Bug 1027734 is about the MozPay API while this issue is caused by the way we expose the MozPaymentProvider API.

The way that this work is something like:

- We have a whitelist of payment providers [1] that defines which flow needs to be loaded in the trusted UI.
- When we get a request via navigator.mozPay(JWT), we process the given JWT and obtain the corresponding payment provider information including its URL.
- Once we have the payment provider information we ask the UI to create the payment flow window (trusted UI) [2]
- On B2G this happen here [3] by sending a mozChromeEvent. On Android it happens somewhere around here [4].
- Focusing on B2G only, Gaia handles this request, creates the iframe to load the payment flow and gives the iframe back to the chrome side via mozContentEvent [6]
- We handle this content event at [7] and this is where we inject the JS shim that contains the implementation of the MozPaymentProvider API [8].

You mentioned via IRC that we could use Func="" to control the exposure of the MozPaymentProvider API, but I am not entirely sure how can we detect that the content where we want to expose this API is actually the payment provider flow. One thing that I guess that we can do is to set a new flag to the trusted UI iframe. Something like "mozpay=true" that could only be set by certified apps. I guess we could then detect this from C++ and allow the exposure of the API or not. Wdyt?

Thanks in advance for your help!

[1] https://mxr.mozilla.org/gaia/source/build/config/payment-prefs.js
[2] https://mxr.mozilla.org/mozilla-central/source/dom/payment/Payment.jsm#392
[3] https://mxr.mozilla.org/mozilla-central/source/b2g/components/PaymentGlue.js#172
[4] https://mxr.mozilla.org/mozilla-central/source/mobile/android/components/PaymentsUI.js#115
[6] https://mxr.mozilla.org/gaia/source/apps/system/js/payment.js#110
[7] https://mxr.mozilla.org/mozilla-central/source/b2g/components/PaymentGlue.js#108
[8] https://mxr.mozilla.org/mozilla-central/source/b2g/chrome/content/payment.js
Assignee: nobody → ferjmoreno
Flags: needinfo?(ferjmoreno) → needinfo?(bobbyholley)
(In reply to Fernando Jiménez Moreno [:ferjm] from comment #15)
> You mentioned via IRC that we could use Func="" to control the exposure of
> the MozPaymentProvider API, but I am not entirely sure how can we detect
> that the content where we want to expose this API is actually the payment
> provider flow. One thing that I guess that we can do is to set a new flag to
> the trusted UI iframe. Something like "mozpay=true" that could only be set
> by certified apps. I guess we could then detect this from C++ and allow the
> exposure of the API or not. Wdyt?

That seems like the most reasonable approach to me, assuming that the other semantics of the API are fixed. I just spoke with sicking, and he agrees. He also suggested that we limit this to the system app (certified + parent process).

You're going to want to add a new enum to FrameType [1], for which we should probably just hardcode that we're in a mozbrowser and inside the system app.

Let me know if this all sounds good and if you need any help. I can also brainstorm JS-only workarounds if we need a faster fix, but doing that securely is always dicey - WebIDL is the right way to go here.

[1] http://mxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.h#782
Flags: needinfo?(bobbyholley)
Blocks: 909896
Duplicate of this bug: 1105524
What is the timeline that we need for the fix here? Where will it need to be backported? Fernando, do you feel confident in getting comment 16 done in a reasonable timeframe, or do we need a stopgap measure?
This blocks Android payments we believe, something we'd hoped to launch this quarter. 2.2 is less of a concern since that's further off.
(In reply to Andy McKay [:andym] from comment #19)
> This blocks Android payments we believe, something we'd hoped to launch this
> quarter. 2.2 is less of a concern since that's further off.

Ok. What release does the fix need to ship in to make tht happen?
I'll try to have something by the end of next week. I'm sorry for not having a better date, but this was quite unexpected.
Summary: Dev payments flow fails to load on 36.0a1 (fxos central) due to JS error → Convert MozPaymentProvider API to WebIDL
(In reply to Bobby Holley (:bholley) from comment #20)
> (In reply to Andy McKay [:andym] from comment #19)
> > This blocks Android payments we believe, something we'd hoped to launch this
> > quarter. 2.2 is less of a concern since that's further off.
> 
> Ok. What release does the fix need to ship in to make tht happen?

I don't know what versions of Android this affects other than nightly, will have to figure out when this landed and which versions are affected as a result.
Bug 1082450 is what broke this, FWIW.
Blocks: 1082450
Keywords: regression
Attached patch Part 0: WebIDL (obsolete) — Splinter Review
Attached patch Part 2: mozpay frame type (obsolete) — Splinter Review
Attached patch Part 3: Payment IDLs (obsolete) — Splinter Review
Attached patch Part 4: DOM (obsolete) — Splinter Review
Attached patch Part 5: B2G (obsolete) — Splinter Review
Duplicate of this bug: 1108544
Blocks: 1109740
Attached patch Part 1: WebIDL (obsolete) — Splinter Review
After talking with Kumar, we are finally exposing mozPaymentProvider in navigator (just like mozPay) instead of the global window. I'll add a patch on the server side to support this on Bug 1109740.

I still need to work on the Android and Desktop pieces and the tests but I would love to hear your feedback about the existing patches in the meantime. Thanks!
Attachment #8533151 - Attachment is obsolete: true
Attachment #8534438 - Flags: feedback?(fabrice)
Attachment #8534438 - Flags: feedback?(bobbyholley)
Attached patch Part 2: mozpay frame type (obsolete) — Splinter Review
This patch adds the mozpay frame property so the MozPaymentProvider API is only exposed to iframes with this property set. I still need to make sure that only the system principal can set this property, but I'd love to hear if you think this patch is going in the right direction.
Attachment #8533152 - Attachment is obsolete: true
Attachment #8534443 - Flags: feedback?(bobbyholley)
Attached patch Part 3: Payment IDLs (obsolete) — Splinter Review
I added a new nsIPaymentProviderStrategy interface that needs to be implemented in each product (just like we do with nsIPaymentUIGlue). We need this because the ICC handling stuff is specific to each product.
Attachment #8533153 - Attachment is obsolete: true
Attachment #8534449 - Flags: feedback?(fabrice)
Attached patch Part 4: DOM (obsolete) — Splinter Review
This patch moves the functionality that we had on the injected payment frame scripts to the DOM implementation.
Attachment #8533154 - Attachment is obsolete: true
Attachment #8534451 - Flags: feedback?(fabrice)
Attached patch Part 5: B2G (obsolete) — Splinter Review
B2G specific bits, including the implementation of the new nsIPaymentProviderStrategy interface.
Attachment #8533155 - Attachment is obsolete: true
Attachment #8534453 - Flags: feedback?(fabrice)
Attached patch Part 8: Gaia (obsolete) — Splinter Review
Attachment #8534467 - Flags: feedback?(fabrice)
Comment on attachment 8534438 [details] [diff] [review]
Part 1: WebIDL

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

::: dom/webidl/MozPaymentProvider.webidl
@@ +10,5 @@
> + NoInterfaceObject,
> + HeaderFile="mozilla/dom/PaymentProvider.h",
> + Func="mozilla::dom::PaymentProvider::EnabledForScope",
> + JSImplementation="@mozilla.org/payment/provider;1"]
> +interface MozPaymentProvider {

Given that this is NoInterfaceObject anyway, i think this should just be called "PaymentProvider", at which point I _think_ the HeaderFile declaration can go away.

@@ +22,5 @@
> +  //       iccId: <string>,
> +  //       dataPrimary: <boolean>
> +  //     },
> +  //    "serviceIdN": {...}
> +  //  }

we should change the serviceId struct to a dictionary, and then make this a sequence of dictionaries.
Attachment #8534438 - Flags: feedback?(bobbyholley) → feedback+
Comment on attachment 8534443 [details] [diff] [review]
Part 2: mozpay frame type

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

Jonas also suggested that we explicitly hardcode these mozpay frames to be part of the system app. I'm a bit on the fence as to whether this is worth it, but this patch doesn't do that.

::: docshell/base/nsDocShell.cpp
@@ +13553,5 @@
> +
> +    nsCOMPtr<nsIDocShellTreeItem> parentAsItem;
> +    GetSameTypeParent(getter_AddRefs(parentAsItem));
> +
> +    nsCOMPtr<nsIDocShell> parent = do_QueryInterface(parentAsItem);

Why do we need all of this? Shouldn't we just be able to do GetInheritedFrameType and be done with it?

::: docshell/base/nsDocShell.h
@@ +803,5 @@
>        }
>      }
>  
>      FrameType GetInheritedFrameType();
> +    bool GetIsInheritedPaymentFrameType();

Why do we need this? Can't everyone just check the result against eFrameTypePayment?

::: docshell/base/nsIDocShell.idl
@@ +856,5 @@
> +   *
> +   * If we don't find a docshell with an associated payment request id in our
> +   * hierarchy, we return NO_PAYMENT_REQUEST_ID.
> +   */
> +  readonly attribute DOMString paymentRequestId;

Do we actually need this? It seems like nobody is using it in the patch. In terms of the original use case for putting this stuff on the docshell, the goal was only to determine whether or not to expose various APIs - so a simple boolean value would suffice. Is there a reason we need more?

::: dom/base/nsFrameLoader.cpp
@@ +1750,5 @@
>      }
>      mDocShell->SetIsBrowserInsideApp(containingAppId);
>    }
>  
> +  if (mOwnerContent->HasAttr(kNameSpaceID_None, nsGkAtoms::mozpay)) {

I think we should check if we have _both_ mozpay and mozid/mozbrowser and throw in that case.

::: dom/payment/PaymentProvider.cpp
@@ +22,5 @@
> +
> +  nsIDocShell *docShell = win->GetDocShell();
> +  if (!docShell) {
> +    return false;
> +  }

Please either use NS_WARN_IF or use the NS_ENSURE_* macros, for both of these.

@@ +25,5 @@
> +    return false;
> +  }
> +
> +  return docShell->GetIsPaymentFrame() ||
> +         docShell->GetIsInPaymentFrame();

Shouldn't the latter be sufficient?

::: dom/payment/moz.build
@@ +26,5 @@
>  
> +FINAL_LIBRARY = 'xul'
> +
> +LOCAL_INCLUDES += [
> +    '/js/xpconnect/wrappers',

Why do you need this part? This is wrong unless I'm missing something.
Attachment #8534443 - Flags: feedback?(bobbyholley)
Thanks for the feedback Bobby!

(In reply to Bobby Holley (:bholley) from comment #37)
> Comment on attachment 8534443 [details] [diff] [review]
> Part 2: mozpay frame type
> 
> Review of attachment 8534443 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Jonas also suggested that we explicitly hardcode these mozpay frames to be
> part of the system app. I'm a bit on the fence as to whether this is worth
> it, but this patch doesn't do that.
> 

Note that this is not only related to B2G but also to Android and WebRT where there is no system app, so we can't hardcode this to be part of the system app only.

This patch didn't have that piece yet, but what I did was adding a check at DocShell::SetIsPaymentFrame to only allow the system app in B2G and the system principal in Android and WebRT to set this frame type.

I'll upload a new patch once I address the rest of your feedback.

> ::: docshell/base/nsDocShell.cpp
> @@ +13553,5 @@
> > +
> > +    nsCOMPtr<nsIDocShellTreeItem> parentAsItem;
> > +    GetSameTypeParent(getter_AddRefs(parentAsItem));
> > +
> > +    nsCOMPtr<nsIDocShell> parent = do_QueryInterface(parentAsItem);
> 
> Why do we need all of this? Shouldn't we just be able to do
> GetInheritedFrameType and be done with it?
> 

You are right. I'm changing this.

> ::: docshell/base/nsDocShell.h
> @@ +803,5 @@
> >        }
> >      }
> >  
> >      FrameType GetInheritedFrameType();
> > +    bool GetIsInheritedPaymentFrameType();
> 
> Why do we need this? Can't everyone just check the result against
> eFrameTypePayment?
> 

Yes, removed.

> ::: docshell/base/nsIDocShell.idl
> @@ +856,5 @@
> > +   *
> > +   * If we don't find a docshell with an associated payment request id in our
> > +   * hierarchy, we return NO_PAYMENT_REQUEST_ID.
> > +   */
> > +  readonly attribute DOMString paymentRequestId;
> 
> Do we actually need this? It seems like nobody is using it in the patch. In
> terms of the original use case for putting this stuff on the docshell, the
> goal was only to determine whether or not to expose various APIs - so a
> simple boolean value would suffice. Is there a reason we need more?
> 

Yes, we need this to let the DOM API implementation know which payment flow belongs to which DOM request. So far we were saving the payment request identifier in the injected frame script [1][2][3], but we don't have that anymore, so we need to save it somewhere else and I thought that this should be the value of the "mozpay" iframe attribute.

[1] https://mxr.mozilla.org/mozilla-central/source/b2g/chrome/content/payment.js#467
[2] https://mxr.mozilla.org/mozilla-central/source/mobile/android/components/PaymentsUI.js#170
[3] https://mxr.mozilla.org/mozilla-central/source/webapprt/PaymentUIGlue.js#123

> ::: dom/base/nsFrameLoader.cpp
> @@ +1750,5 @@
> >      }
> >      mDocShell->SetIsBrowserInsideApp(containingAppId);
> >    }
> >  
> > +  if (mOwnerContent->HasAttr(kNameSpaceID_None, nsGkAtoms::mozpay)) {
> 
> I think we should check if we have _both_ mozpay and mozid/mozbrowser and
> throw in that case.

Ok, done.
 
> ::: dom/payment/PaymentProvider.cpp
> @@ +22,5 @@
> > +
> > +  nsIDocShell *docShell = win->GetDocShell();
> > +  if (!docShell) {
> > +    return false;
> > +  }
> 
> Please either use NS_WARN_IF or use the NS_ENSURE_* macros, for both of
> these.
> 

Done.

> @@ +25,5 @@
> > +    return false;
> > +  }
> > +
> > +  return docShell->GetIsPaymentFrame() ||
> > +         docShell->GetIsInPaymentFrame();
> 
> Shouldn't the latter be sufficient?
>

Yes, I've changed it.
 
> ::: dom/payment/moz.build
> @@ +26,5 @@
> >  
> > +FINAL_LIBRARY = 'xul'
> > +
> > +LOCAL_INCLUDES += [
> > +    '/js/xpconnect/wrappers',
> 
> Why do you need this part? This is wrong unless I'm missing something.

Yeah, this was wrong. Old code. I removed it.
(In reply to Fernando Jiménez Moreno [:ferjm] from comment #38)
> > ::: docshell/base/nsIDocShell.idl
> > @@ +856,5 @@
> > > +   *
> > > +   * If we don't find a docshell with an associated payment request id in our
> > > +   * hierarchy, we return NO_PAYMENT_REQUEST_ID.
> > > +   */
> > > +  readonly attribute DOMString paymentRequestId;
> > 
> > Do we actually need this? It seems like nobody is using it in the patch. In
> > terms of the original use case for putting this stuff on the docshell, the
> > goal was only to determine whether or not to expose various APIs - so a
> > simple boolean value would suffice. Is there a reason we need more?
> > 
> 
> Yes, we need this to let the DOM API implementation know which payment flow
> belongs to which DOM request. So far we were saving the payment request
> identifier in the injected frame script [1][2][3], but we don't have that
> anymore, so we need to save it somewhere else and I thought that this should
> be the value of the "mozpay" iframe attribute.
> 
> [1]
> https://mxr.mozilla.org/mozilla-central/source/b2g/chrome/content/payment.
> js#467
> [2]
> https://mxr.mozilla.org/mozilla-central/source/mobile/android/components/
> PaymentsUI.js#170
> [3]
> https://mxr.mozilla.org/mozilla-central/source/webapprt/PaymentUIGlue.js#123
> 

This is use here btw https://bugzilla.mozilla.org/attachment.cgi?id=8534451&action=diff#a/dom/payment/PaymentProvider.js_sec2 Line 86
Attached patch Part 2: mozpay frame type. v2 (obsolete) — Splinter Review
Bobby, could you take another look, please? I think I addressed all your previous feedback or replied to your questions at comment 38. Thanks for your help!
Attachment #8534443 - Attachment is obsolete: true
Comment on attachment 8535703 [details] [diff] [review]
Part 2: mozpay frame type. v2

Sorry, I forgot to set the f? flag.
Attachment #8535703 - Flags: feedback?(bobbyholley)
Attached patch Part 6: Android (WIP) (obsolete) — Splinter Review
Comment on attachment 8535705 [details] [diff] [review]
Part 6: Android (WIP)

Mark, I am trying to fix the Android bits as well, but I am a bit blocked with this. As you can see by reading the previous comments, the idea is to only expose the MozPaymentProvider API on iframes with a "mozpay" attribute. We want to limit who is able to set this iframe attribute. In B2G is clear that we can limit it to the system app. I thought that we could limit it also to the system principal in Android, but it seems that this is not the case with the approach I am currently following. I am loading about:blank and creating an iframe with the mozpay attribute from the PaymentUI.jsm module, but if I am not wrong this new tab I am creating is unprivileged content and so it can't set this attribute.

Bobby, I am also adding the f? on you just in case you can think about an alternative approach to limit who can set the mozpay attribute here.

Thanks!
Attachment #8535705 - Flags: feedback?(mark.finkle)
Attachment #8535705 - Flags: feedback?(bobbyholley)
Comment on attachment 8535705 [details] [diff] [review]
Part 6: Android (WIP)

Wes has more experience with mozpay than I do.
Attachment #8535705 - Flags: feedback?(mark.finkle) → feedback?(wjohnston)
Attachment #8534438 - Flags: feedback?(fabrice) → feedback+
Attachment #8534449 - Flags: feedback?(fabrice) → feedback+
Comment on attachment 8534451 [details] [diff] [review]
Part 4: DOM

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

In PaymentProvider.js, the amount of #ifdef makes the flow hard to follow. Can you try to extract the |#if defined(MOZ_B2G_RIL) || defined(MOZ_WIDGET_ANDROID)| parts to single block or file?

::: dom/payment/PaymentProvider.js
@@ +27,5 @@
> +} catch(e){
> +  _debug = false;
> +}
> +
> +function LOG(s) {

s/LOG/DEBUG ?
Attachment #8534451 - Flags: feedback?(fabrice)
Comment on attachment 8534453 [details] [diff] [review]
Part 5: B2G

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

::: b2g/components/PaymentGlue.js
@@ +7,5 @@
>  const { classes: Cc, interfaces: Ci, utils: Cu } = Components;
>  
>  Cu.import("resource://gre/modules/XPCOMUtils.jsm");
>  Cu.import("resource://gre/modules/Services.jsm");
> +Cu.import("resource://gre/modules/Promise.jsm");

You can use the native promise support instead.

::: b2g/components/PaymentProviderStrategy.js
@@ +77,5 @@
> +    }
> +
> +    gSettingsService.createLock().set(kRilDefaultPaymentServiceId,
> +                                      serviceId, null);
> +    this._paymentServiceId = serviceId;

So it's ok to have a null serviceId? What does that mean?

@@ +111,5 @@
> +      return;
> +    }
> +
> +    try {
> +      if ('wrappedJSObject' in aSubject) {

nit: double quotes everywhere else in this file.
Attachment #8534453 - Flags: feedback?(fabrice) → feedback+
Attachment #8534467 - Flags: feedback?(fabrice) → feedback+
Comment on attachment 8535703 [details] [diff] [review]
Part 2: mozpay frame type. v2

Nevermind. I am getting rid of the "mozpay" iframe attribute as I think we don't need it.
Attachment #8535703 - Flags: feedback?(bobbyholley)
Attachment #8535705 - Flags: feedback?(wjohnston)
Attachment #8535705 - Flags: feedback?(bobbyholley)
Blocks: 1112052
Attached patch Part 1: WebIDLSplinter Review
Attachment #8534438 - Attachment is obsolete: true
Attachment #8537796 - Flags: review?(bobbyholley)
Instead of adding the mozpay iframe attribute, we can set the payment request ID in the payment's iframe docshell directly.
Attachment #8535703 - Attachment is obsolete: true
Attachment #8537798 - Flags: review?(bobbyholley)
Attachment #8534449 - Attachment is obsolete: true
Attachment #8537799 - Flags: review?(fabrice)
Attached patch Part 4: DOM (obsolete) — Splinter Review
Attachment #8534451 - Attachment is obsolete: true
Attachment #8537801 - Flags: review?(fabrice)
Attached patch Part 5: B2G (obsolete) — Splinter Review
Attachment #8534453 - Attachment is obsolete: true
Attachment #8537802 - Flags: review?(fabrice)
Attached patch Part 6: Android (obsolete) — Splinter Review
Wes, we are changing the way the MozPaymentProvider API is exposed. Instead of manually injecting it in the payment window and exposing its methods via __exposedProps__, we are exposing it depending on the existence of the payment request id in the payment's window docshell.
Attachment #8535705 - Attachment is obsolete: true
Attachment #8537803 - Flags: review?(wjohnston)
Comment on attachment 8534467 [details] [diff] [review]
Part 8: Gaia

We don't need a gaia patch anymore
Attachment #8534467 - Attachment is obsolete: true
I am working on the tests for the other pieces.
Attachment #8537805 - Flags: review?(bobbyholley)
Comment on attachment 8537796 [details] [diff] [review]
Part 1: WebIDL

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

These patches aren't "parts", because they won't compile without each other, right? Make sure to land them as one changeset so as not to break bisection.

::: dom/webidl/MozPaymentProvider.webidl
@@ +29,5 @@
> +  //       iccId: <string>,
> +  //       dataPrimary: <boolean>
> +  //     },
> +  //    "serviceIdN": {...}
> +  //  }

I think the second part of this comment is basically redundant with the declared type and the dictionary definition above. I'd prefer to remove it.

@@ +30,5 @@
> +  //       dataPrimary: <boolean>
> +  //     },
> +  //    "serviceIdN": {...}
> +  //  }
> +  [Cached, Pure] readonly attribute sequence<PaymentIccInfo> iccInfo;

This should also be [Frozen], I think.
Attachment #8537796 - Flags: review?(bobbyholley) → review+
Comment on attachment 8537798 [details] [diff] [review]
Part 2: DocShell payment request id

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

r=me with those changes.

::: docshell/base/nsDocShell.cpp
@@ +13717,5 @@
> +NS_IMETHODIMP
> +nsDocShell::GetPaymentRequestId(nsAString& aPaymentRequestId)
> +{
> +    aPaymentRequestId = mPaymentRequestId;
> +    return NS_OK;

So I presume you no longer care about this propagating to child docshells?

::: docshell/base/nsIDocShell.idl
@@ +1042,5 @@
>     */
>    [infallible] readonly attribute boolean hasLoadedNonBlankURI;
>  
> +  /**
> +   * Holds the id of the payment request associated with this docshell.

(if any)

::: dom/payment/PaymentProviderEnabler.cpp
@@ +10,5 @@
> +
> +using namespace mozilla::dom;
> +
> +/* static */ bool
> +PaymentProviderEnabler::EnabledForScope(JSContext* aCx,

I think that this file should be called PaymentProviderUtils.{cpp,h}, and that the function should be:

mozilla::dom::PaymentProviderEnabledForScope(...)

@@ +15,5 @@
> +			                                  JSObject* aGlobal)
> +{
> +  nsCOMPtr<nsPIDOMWindow> win =
> +    do_QueryInterface(nsJSUtils::GetStaticScriptGlobal(aGlobal));
> +  NS_ENSURE_TRUE(win, false);

Please do xpc::WindowOrNull.
Attachment #8537798 - Flags: review?(bobbyholley) → review+
Comment on attachment 8537805 [details] [diff] [review]
Part 2.1: Test MozPaymentProvider exposure

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

::: dom/payment/tests/mochitest/file_mozpayproviderchecker.html
@@ +17,5 @@
> +    let exposed = (navigator.mozPaymentProvider != undefined);
> +    window.parent.postMessage(JSON.stringify({
> +      iframeType: message.iframeType,
> +      exposed: exposed
> +    }), "*");

postMessage accepts objects - no need to stringify/parse.

::: dom/payment/tests/mochitest/test_mozpaymentprovider.html
@@ +50,5 @@
> +    let docshell = SpecialPowers.wrap(paymentIframe.contentWindow)
> +                                .QueryInterface(Ci.nsIInterfaceRequestor)
> +                                .getInterface(Ci.nsIWebNavigation)
> +                                .QueryInterface(Ci.nsIDocShell);
> +    docshell.paymentRequestId = "dummyid";

This is wrong. In order to be guaranteed that the API shows up, this flag needs to be set on the docshell before the load happens - so after the iframe is appended to the document, but _before_ src is set.
Attachment #8537805 - Flags: review?(bobbyholley) → review+
Actually, we need a Gaia patch. Now we set the src of the payment iframe *after* setting the payment request ID in the docshell, as mentioned in comment 58.
Attachment #8538482 - Flags: review?(fabrice)
Attached patch Part 5: B2GSplinter Review
Now we set the payment iframe src in the chrome side for B2G as well after the docshell is flagged with the payment details.
Attachment #8537802 - Attachment is obsolete: true
Attachment #8537802 - Flags: review?(fabrice)
Attachment #8538551 - Flags: review?(fabrice)
Comment on attachment 8537803 [details] [diff] [review]
Part 6: Android

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

This seems good to me. Nice :)

::: mobile/android/app/mobile.js
@@ +775,5 @@
>  pref("dom.payment.provider.0.description", "marketplace.firefox.com");
>  pref("dom.payment.provider.0.uri", "https://marketplace.firefox.com/mozpay/?req=");
>  pref("dom.payment.provider.0.type", "mozilla/payments/pay/v1");
>  pref("dom.payment.provider.0.requestMethod", "GET");
> +pref('dom.payment.provider.1.name', 'mockpayprovider');

I assume this is for testing? Can you add a note in the prefs file about this. In fact, maybe this should just be in the robocop testing prefs file?
Attachment #8537803 - Flags: review?(wjohnston) → review+
Thank you Wes.

(In reply to Wesley Johnston (:wesj) from comment #61)
> @@ +775,5 @@
> >  pref("dom.payment.provider.0.description", "marketplace.firefox.com");
> >  pref("dom.payment.provider.0.uri", "https://marketplace.firefox.com/mozpay/?req=");
> >  pref("dom.payment.provider.0.type", "mozilla/payments/pay/v1");
> >  pref("dom.payment.provider.0.requestMethod", "GET");
> > +pref('dom.payment.provider.1.name', 'mockpayprovider');
> 
> I assume this is for testing? Can you add a note in the prefs file about
> this. In fact, maybe this should just be in the robocop testing prefs file?

Yeah, actually this needs to go away. It was only there for my local tests.
Attachment #8540074 - Flags: review?(fabrice)
Comment on attachment 8537799 [details] [diff] [review]
Part 3: Payment IDLs

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

::: dom/payment/interfaces/nsIPaymentUIGlue.idl
@@ +25,5 @@
>      void showPaymentFlow(in DOMString requestId,
>                           in nsIPaymentFlowInfo paymentFlowInfo,
>                           in nsIPaymentUIGlueCallback errorCb);
>  
> +    jsval /*Promise*/ closePaymentFlow(in DOMString requestId);

nit: document what the promise resolves to.
Attachment #8537799 - Flags: review?(fabrice) → review+
Comment on attachment 8537801 [details] [diff] [review]
Part 4: DOM

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

I'd like to be sure about the observer & the runnable.

::: dom/payment/PaymentProvider.js
@@ +60,5 @@
> +
> +  paymentSuccess: function(aResult) {
> +    if (_debug) {
> +      DEBUG("paymentSuccess " + aResult);
> +    }

nit: for terseness, you can switch all your debug guards to single line: |_debug && DEBUG("...");|

@@ +77,5 @@
> +    if (_debug) {
> +      DEBUG("paymentFailed " + aError);
> +    }
> +    let glue = Cc["@mozilla.org/payment/ui-glue;1"]
> +               .createInstance(Ci.nsIPaymentUIGlue);

nit: align .createInstance with ["..

@@ +119,5 @@
> +      DEBUG("Cleaning up!");
> +    }
> +
> +    this._strategy.cleanup();
> +    Services.obs.removeObserver(this.oncancel, kPaymentFlowCancelled);

You add the observer with this.oncancel.bind(this), is that gonna remove it properly?
If that's the only topic we observe, just addObserver(this) ?

@@ +188,5 @@
> +                                           "NO_PAYMENT_SERVICE_ID");
> +      }
> +    };
> +    Services.tm.currentThread.dispatch(runnable,
> +                                       Ci.nsIThread.DISPATCH_NORMAL);

Why do you need to dispatch yourself since you already use fireErrorAsync?

::: dom/payment/moz.build
@@ +14,5 @@
>      'PaymentProviderEnabler.cpp',
>  ]
>  
>  EXTRA_PP_JS_MODULES += [
> +    'Payment.jsm'

nit: no need for this change

@@ +26,5 @@
>  
> +EXTRA_PP_COMPONENTS += [
> +    'PaymentProvider.js'
> +]
> + 

nit: extra whitespace
Attachment #8537801 - Flags: review?(fabrice) → feedback+
Comment on attachment 8538482 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/27557

lgtm
Attachment #8538482 - Flags: review?(fabrice) → review+
Comment on attachment 8538551 [details] [diff] [review]
Part 5: B2G

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

::: b2g/components/PaymentGlue.js
@@ +141,5 @@
>      SystemAppProxy.dispatchEvent(detail);
>    },
>  
> +  closePaymentFlow: function(aRequestId) {
> +    return new Promise((resolve, reject) => {

nit: aResolve, aReject
Attachment #8538551 - Flags: review?(fabrice) → review+
Attachment #8540074 - Flags: review?(fabrice) → review+
Attached patch Part 4: DOMSplinter Review
Thanks Fabrice! Nice catch with the observers thing. We are observing for more than one topic, so I needed to save the bound observers to be able to remove them later.
Attachment #8537801 - Attachment is obsolete: true
Attachment #8541487 - Flags: review?(fabrice)
Attached patch Part 6: Android (obsolete) — Splinter Review
Wes, I had to modify the patch slightly. Now we are doing the same as B2G does. We set the payment information in the docshell *before* loading the payment provider flow. We couldn't do this with BrowserApp.addTab directly, so I added a new chrome xhtml page to wrap the payment flow iframe that we can open without opening the actual payment provider url.
Attachment #8537803 - Attachment is obsolete: true
Attachment #8541505 - Flags: review?(wjohnston)
Depends on: 1117131
Comment on attachment 8541505 [details] [diff] [review]
Part 6: Android

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

The remote stuff made everyone nervous when I asked. Is this tested? Removing the "remote" bit would make me a bit less nervous. I'll hold off on r+ until that's answered. Things like long pressing on links or test may cause crashes? Landscape mode so that panning is required? I'm surprised if clicking works tbh, but maybe...

::: mobile/android/components/PaymentsUI.js
@@ +103,5 @@
> +      tab.browser.removeEventListener("DOMContentLoaded", onloaded);
> +      let document = tab.browser.contentDocument;
> +      let frame = document.getElementById("payflow");
> +      frame.setAttribute("mozbrowser", true);
> +      frame.setAttribute("remote", true);

I am a bit surprised if this works (since we haven't remoted so much of Fennec at this point).... I would remove it unless you're sure its necessary.

::: mobile/android/themes/core/payment.css
@@ +3,5 @@
> +  position: absolute;
> +  top: 0;
> +  left: 0;
> +  right: 0;
> +  bottom: 0;

Why do you absolutely position this? Scrolling actually going to suck in Fennec (since we don't have good iframe scrolling yet), but I doubt there's much scrolling here anyway. seamless would work well if we supported it.
Attachment #8541505 - Flags: review?(wjohnston) → review-
(In reply to Wesley Johnston (:wesj) from comment #70)

> The remote stuff made everyone nervous when I asked. Is this tested?
> Removing the "remote" bit would make me a bit less nervous. I'll hold off on
> r+ until that's answered. Things like long pressing on links or test may
> cause crashes? Landscape mode so that panning is required? I'm surprised if
> clicking works tbh, but maybe...
> 
> ::: mobile/android/components/PaymentsUI.js
> @@ +103,5 @@
> > +      tab.browser.removeEventListener("DOMContentLoaded", onloaded);
> > +      let document = tab.browser.contentDocument;
> > +      let frame = document.getElementById("payflow");
> > +      frame.setAttribute("mozbrowser", true);
> > +      frame.setAttribute("remote", true);
> 
> I am a bit surprised if this works (since we haven't remoted so much of
> Fennec at this point).... I would remove it unless you're sure its necessary.
> 

Yes, it is tested and it works fine. I removed the "remote" anyway. This is required in b2g cause the trusted UI is part of the system app, but I guess it is ok to leave it in process for android.

> ::: mobile/android/themes/core/payment.css
> @@ +3,5 @@
> > +  position: absolute;
> > +  top: 0;
> > +  left: 0;
> > +  right: 0;
> > +  bottom: 0;
> 
> Why do you absolutely position this? Scrolling actually going to suck in
> Fennec (since we don't have good iframe scrolling yet), but I doubt there's
> much scrolling here anyway. seamless would work well if we supported it.

I just need the iframe to fill the screen. I changed it from absolute to fixed. Let me know if there is a better way to do that.

/me css ignorant :|

Thanks Wes!
Attached patch Part 6: AndroidSplinter Review
Attachment #8541505 - Attachment is obsolete: true
Attachment #8544448 - Flags: review?(wjohnston)
Attachment #8541487 - Flags: review?(fabrice) → review+
Attachment #8544448 - Flags: review?(wjohnston) → review+
ferjm: I was chatting to fabrice and he said we need to mark patches on per patch basis for 2.2 approval. I believe some of these will need uplifted to 2.2?
Flags: needinfo?(ferjmoreno)
Component: Payments/Refunds → DOM
Flags: needinfo?(ferjmoreno)
Product: Marketplace → Core
Version: 1.4 → Trunk
Comment on attachment 8537796 [details] [diff] [review]
Part 1: WebIDL

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 1082450
User impact if declined: Payments won't work.
Testing completed: Manual testing and unit tests added.
Risk to taking this patch (and alternatives if risky): Give that payments can't hardly be more broken than now, very low risk.
String or UUID changes made by this patch: No strings but UUID changes for docshell/base/nsIDocShell.idl and dom/payment/interfaces/nsIPaymentUIGlue.idl
Attachment #8537796 - Flags: approval-mozilla-b2g37?
Attachment #8537798 - Flags: approval-mozilla-b2g37?
Attachment #8537799 - Flags: approval-mozilla-b2g37?
Attachment #8537805 - Flags: approval-mozilla-b2g37?
Attachment #8538551 - Flags: approval-mozilla-b2g37?
Attachment #8540074 - Flags: approval-mozilla-b2g37?
Attachment #8541487 - Flags: approval-mozilla-b2g37?
Attachment #8544448 - Flags: approval-mozilla-b2g37?
Attachment #8538482 - Flags: approval-mozilla-b2g37?
Attachment #8552322 - Flags: approval-mozilla-b2g37?
Attachment #8538482 - Attachment description: Part 7: Gaia → Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/27557
Attachment #8538482 - Flags: approval-mozilla-b2g37?
Attachment #8537796 - Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+
Attachment #8537798 - Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+
Attachment #8537799 - Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+
Attachment #8537805 - Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+
Attachment #8538551 - Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+
Attachment #8540074 - Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+
Attachment #8541487 - Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+
Attachment #8544448 - Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+
Attachment #8552322 - Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+
Clearing the blocking nom for 2.2? as this is already fixed/verified on that branch per the status flag
blocking-b2g: 2.2? → ---
Comment on attachment 8537796 [details] [diff] [review]
Part 1: WebIDL

Approval Request Comment
[Feature/regressing bug #]: Bug 1082450
[User impact if declined]: Payments won't work
[Describe test coverage new/current, TreeHerder]: Manual testing and unit tests added.
[Risks and why]: The common part of this set of patches has been tested for a few weeks already on b2g without any regression. The specific android part is quite self contained and it shouldn't affect any other piece of fennec.
[String/UUID change made/needed]: No strings but UUID changes for docshell/base/nsIDocShell.idl and dom/payment/interfaces/nsIPaymentUIGlue.idl
Attachment #8537796 - Flags: approval-mozilla-beta?
Attachment #8537798 - Flags: approval-mozilla-beta?
Attachment #8537799 - Flags: approval-mozilla-beta?
Attachment #8537805 - Flags: approval-mozilla-beta?
Attachment #8538551 - Flags: approval-mozilla-beta?
Attachment #8540074 - Flags: approval-mozilla-beta?
Attachment #8541487 - Flags: approval-mozilla-beta?
Attachment #8544448 - Flags: approval-mozilla-beta?
Hey there Ferjm - 

Can you pls provide info on the following and then re-nom the bug ( status-firefox37:? )
- what's the risk this patch will break fennec?
- what's the risk it will break apps that people already have / existing on their mobile devices?

Spoke with Lawrence Mandel (release manager, added to bug) and we need to get and understanding of the technical risk as well as the bug nom'd asap in order to be re-considered for Friday's uplift cutoff for Mobile. 


Thanks so much for providing more info Ferjm!
Flags: needinfo?(ferjmoreno)
:lmandel

Here is some more info on the "why uplift"

https://wiki.mozilla.org/Release_Management/Feature_Uplift

1) Has a commitment been made to release the feature by a certain date?
internally yes. this feature was live, and was promised to our business development team for conversations with content partners. when it broke, there was an internal commitment to fix asap (the priority of firefox accounts delayed the fix)

2) Is the feature part of a cross product, coordinated release?
yes: marketplace payments has dependencies with marketplace, firefox accounts and each platform (OS, Desktop, Mobile)

3) Is the feature related to a time based market or partner opportunity?
yes: FxOS Marketplace is in the midst of a significant push to get app partners onboard FxOS via our Mobile and Desktop channels. Content partners are very interested in the expanded audience of Desktop and Mobile, but want to come on board with monetization (payments) already in place on all platforms (desktop and OS both have payments)
(In reply to Caitlin Galimidi [:c8o] from comment #82)
> Hey there Ferjm - 
> 
> Can you pls provide info on the following and then re-nom the bug (
> status-firefox37:? )

I asked for approval-mozilla-beta? for each of the patches as well. I can't check if it applies well on top of mozilla-beta until tomorrow though.

> - what's the risk this patch will break fennec?

As I mentioned in comment 81, the risk is very low.

The common part of these patches that affects all the products where payments are enabled is a tinny change in DocShell that adds a getter and a setter that no other code apart from the payments implementation is using. It has been tested for a few weeks already in B2G without any regression so far.

The specific part for fennec is also only affecting payments. We added a new chrome page and modified the already existing payments code. It should not affect the stability of fennec or any of its other features.

> - what's the risk it will break apps that people already have / existing on
> their mobile devices?
> 

Very low risk, for the same reasons above.

Please, let me know if you need any further info.
Flags: needinfo?(ferjmoreno)
After reviewing the changes, I do see that the changes are for the most part isolated. The one concern that I have is the UUID bump on nsIDocShell.idl, which mconley tells me does have a good chance of being in use by binary add-ons (in which case this change will break them). 

ferjm - Is there a way to fix payments in 37 without modifying this interface? If so, that seems preferable. If not, I would prefer to better understand the potential impact but don't think we have that data. If we go ahead with the interface change, we'll need to inform add-on authors of the change.

ni Jorge so that he's aware that we're considering this change for 37.
Flags: needinfo?(jorge)
Flags: needinfo?(ferjmoreno)
I would also prefer that this change is implemented differently for 37 if possible. Changing nsIDocShell seems likely to break binary add-ons.
Flags: needinfo?(jorge)
I can try a nasty hack hardcoding the list of allowed origins.
Flags: needinfo?(ferjmoreno)
(In reply to Fernando Jiménez Moreno [:ferjm] from comment #87)
> I can try a nasty hack hardcoding the list of allowed origins.

Depending on how nasty the hack and how much time this will take and that this may not be complete by Beta 4 (gtb today), I think that we should consider whether it is simple safer/better to ship this fix in 38. I understand the market pressure but if we're forcing this feature too much (next mobile Beta gtb is next Monday for Beta 6, second to last mobile Beta this cycle) it may be better to let the proper and tested implementation ship in the next cycle.
Do we also need to port this the Desktop Webapp RT? Assuming we can just apply those patches cleanly to that tree?
Flags: needinfo?(ferjmoreno)
(In reply to Lawrence Mandel [:lmandel] (use needinfo) from comment #88)
> (In reply to Fernando Jiménez Moreno [:ferjm] from comment #87)
> > I can try a nasty hack hardcoding the list of allowed origins.
> 
> Depending on how nasty the hack and how much time this will take and that
> this may not be complete by Beta 4 (gtb today), I think that we should
> consider whether it is simple safer/better to ship this fix in 38. I
> understand the market pressure but if we're forcing this feature too much
> (next mobile Beta gtb is next Monday for Beta 6, second to last mobile Beta
> this cycle) it may be better to let the proper and tested implementation
> ship in the next cycle.

I think the hack would be quite nasty cause I would need to hardcode all the urls that should have access to navigator.mozPay. We currently have the url of the Mozilla payment provider whitelisted in Gecko, but I think that's not the only url that accesses navigator.mozPay cause the payment provider does some redirections. And in any case I don't know how could I expose the API to child iframes with this hack, so the payment provider would need to do all the mozPay calls from top level windows, which I don't know if that's the current case.

In summary, I believe this kind of hack would be riskier as it would depend on these remote URLs been the same and the payment provider not changing their implementation.
Flags: needinfo?(ferjmoreno) → needinfo?(lmandel)
(In reply to Andy McKay [:andym] from comment #89)
> Do we also need to port this the Desktop Webapp RT? Assuming we can just
> apply those patches cleanly to that tree?

I didn't fix the Desktop Webapp RT implementation with these patches, only B2G and Android. The bug for tracking the Desktop Webapp RT implementation is bug 1134007.
Comment on attachment 8537796 [details] [diff] [review]
Part 1: WebIDL

Given Jorge and my concerns about breaking add-ons by changing the X interface mid Beta cycle (comment 85, comment 86), the risk of a hack that doesn't involve changing this interface (comment 90), and that our users will have no immediate benefit from this change, I think that it is in the best interest of our users and the 37 release to deny uplift for this feature and ship it in 38, where it has already landed. Firefox 38 is scheduled to release on May 12, 2015. Beta-
Flags: needinfo?(lmandel)
Attachment #8537796 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
s/nsIDocShell.idl/X/
Attachment #8537798 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
Attachment #8537799 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
Attachment #8537805 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
Attachment #8538551 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
Attachment #8540074 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
Attachment #8541487 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
Attachment #8544448 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.