1097928 - Convert MozPaymentProvider API to WebIDL

Status: RESOLVED FIXED

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

Description

[krupa raj[:krupa]]

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

Comment 1

[Andy McKay]

Sounds like the cause might be bug 1084245

Comment 2

[Andrew Sutherland [:asuth] (he/him)]

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!

Comment 3

[[e-mail]]

I'll take a look and see if I can find what part of our code is triggering this early next week.

Comment 4

[[e-mail]]

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.

Comment 5

[[e-mail]]

I get the same as comment4 with the original build noted in the STR.

Comment 6

[[e-mail]]

Raised a bug for the other issue as per comment 4: bug 1101132

Comment 7

[Iulian Timis]

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

Comment 8

[[e-mail]]

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

Comment 9

[[e-mail]]

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}]

Comment 10

[[e-mail]]

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

Comment 11

[Andy McKay]

bholley, would this be related to bug 1084245 (and a few other bugs around that)?

Comment 12

[Bobby Holley (:bholley)]

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.

Comment 13

[Andy McKay]

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

Comment 14

[Bobby Holley (:bholley)]

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

Comment 15

[Fernando Jiménez Moreno [:ferjm]]

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

Comment 16

[Bobby Holley (:bholley)]

(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

Comment 18

[Bobby Holley (:bholley)]

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?

Comment 19

[Andy McKay]

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.

Comment 20

[Bobby Holley (:bholley)]

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

Comment 21

[Fernando Jiménez Moreno [:ferjm]]

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.

Comment 22

[Andy McKay]

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

Comment 23

[Bobby Holley (:bholley)]

Bug 1082450 is what broke this, FWIW.

Comment 24

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 0: WebIDL

Comment 25

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 2: mozpay frame type

Comment 26

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 3: Payment IDLs

Comment 27

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 4: DOM

Comment 28

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 5: B2G

Comment 30

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 1: WebIDL

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!

Comment 31

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 2: mozpay frame type

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.

Comment 32

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 3: Payment IDLs

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.

Comment 33

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 4: DOM

This patch moves the functionality that we had on the injected payment frame scripts to the DOM implementation.

Comment 34

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 5: B2G

B2G specific bits, including the implementation of the new nsIPaymentProviderStrategy interface.

Comment 35

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 8: Gaia

Comment 36

[Bobby Holley (:bholley)]

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.

Comment 37

[Bobby Holley (:bholley)]

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.

Comment 38

[Fernando Jiménez Moreno [:ferjm]]

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.

Comment 39

[Fernando Jiménez Moreno [:ferjm]]

(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

Comment 40

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 2: mozpay frame type. v2

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!

Comment 41

[Fernando Jiménez Moreno [:ferjm]]

Comment on attachment 8535703 [details] [diff] [review]
Part 2: mozpay frame type. v2

Sorry, I forgot to set the f? flag.

Comment 42

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 6: Android (WIP)

Comment 43

[Fernando Jiménez Moreno [:ferjm]]

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!

Comment 44

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 8535705 [details] [diff] [review]
Part 6: Android (WIP)

Wes has more experience with mozpay than I do.

Comment 45

[[:fabrice] Fabrice Desré]

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 ?

Comment 46

[[:fabrice] Fabrice Desré]

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.

Comment 47

[Fernando Jiménez Moreno [:ferjm]]

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.

Comment 48

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 1: WebIDL

Comment 49

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 2: DocShell payment request id

Instead of adding the mozpay iframe attribute, we can set the payment request ID in the payment's iframe docshell directly.

Comment 50

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 3: Payment IDLs

Comment 51

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 4: DOM

Comment 52

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 5: B2G

Comment 53

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 6: Android

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.

Comment 54

[Fernando Jiménez Moreno [:ferjm]]

Comment on attachment 8534467 [details] [diff] [review]
Part 8: Gaia

We don't need a gaia patch anymore

Comment 55

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 2.1: Test MozPaymentProvider exposure

I am working on the tests for the other pieces.

Comment 56

[Bobby Holley (:bholley)]

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.

Comment 57

[Bobby Holley (:bholley)]

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.

Comment 58

[Bobby Holley (:bholley)]

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.

Comment 59

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/27557

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.

Comment 60

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 5: B2G

Now we set the payment iframe src in the chrome side for B2G as well after the docshell is flagged with the payment details.

Comment 61

[Wesley Johnston (:wesj)]

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?

Comment 62

[Fernando Jiménez Moreno [:ferjm]]

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.

Comment 63

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 5.1: DOM Tests

Comment 64

[[:fabrice] Fabrice Desré]

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.

Comment 65

[[:fabrice] Fabrice Desré]

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

Comment 66

[[:fabrice] Fabrice Desré]

Comment on attachment 8538482 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/27557

lgtm

Comment 67

[[:fabrice] Fabrice Desré]

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

Comment 68

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 4: DOM

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.

Comment 69

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 6: Android

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.

Comment 70

[Wesley Johnston (:wesj)]

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.

Comment 71

[Fernando Jiménez Moreno [:ferjm]]

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

Comment 72

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 6: Android

Comment 73

[Fernando Jiménez Moreno [:ferjm]]

https://hg.mozilla.org/integration/b2g-inbound/rev/bbd37e7c0d08
https://hg.mozilla.org/integration/b2g-inbound/rev/6ea82f73b2b4

Comment 74

[Fernando Jiménez Moreno [:ferjm]]

Treeherder is finally happy

https://treeherder.mozilla.org/ui/#/jobs?repo=gaia-try&revision=751435095bfc

Gaia master: https://github.com/mozilla-b2g/gaia/commit/8cf15f8bc015f0cc4865f2962b711c91ff69da1d

Comment 75

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/bbd37e7c0d08
https://hg.mozilla.org/mozilla-central/rev/6ea82f73b2b4

Comment 76

[Andy McKay]

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?

Comment 77

[Fernando Jiménez Moreno [:ferjm]]

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

Comment 78

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Gaia patch rebased for v2.2

Comment 79

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/bb2328933acd
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/4dd4af1f59f9

v2.2: https://github.com/mozilla-b2g/gaia/commit/237008137f6d72b9cad25ff4faff14ff2c40ac63

Comment 80

[bhavana bajaj [:bajaj]]

Clearing the blocking nom for 2.2? as this is already fixed/verified on that branch per the status flag

Comment 81

[Fernando Jiménez Moreno [:ferjm]]

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

Comment 82

[Caitlin Galimidi [:c8o]]

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!

Comment 83

[Caitlin Galimidi [:c8o]]

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

Comment 84

[Fernando Jiménez Moreno [:ferjm]]

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

Comment 85

[Lawrence Mandel [:lmandel] (use needinfo)]

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.

Comment 86

[Jorge Villalobos [:jorgev] (he/him)]

I would also prefer that this change is implemented differently for 37 if possible. Changing nsIDocShell seems likely to break binary add-ons.

Comment 87

[Fernando Jiménez Moreno [:ferjm]]

I can try a nasty hack hardcoding the list of allowed origins.

Comment 88

[Lawrence Mandel [:lmandel] (use needinfo)]

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

Comment 89

[Andy McKay]

Do we also need to port this the Desktop Webapp RT? Assuming we can just apply those patches cleanly to that tree?

Comment 90

[Fernando Jiménez Moreno [:ferjm]]

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

Comment 91

[Fernando Jiménez Moreno [:ferjm]]

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

[Lawrence Mandel [:lmandel] (use needinfo)]

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-

Comment 93

[Lawrence Mandel [:lmandel] (use needinfo)]

s/nsIDocShell.idl/X/