Closed Bug 1477409 Opened Last year Closed Last year

Pass outerWindowId to paymentUIService.showPayment

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla65
Tracking Status
firefox63 --- wontfix
firefox64 --- wontfix
firefox65 --- fixed

People

(Reporter: jaws, Assigned: edenchuang)

References

(Blocks 1 open bug)

Details

(Whiteboard: [webpayments-reserve])

Attachments

(2 files, 3 obsolete files)

Right now we use Services.wm.getMostRecentWindow[1] to get a reference to a window from which we will open the payments dialog. This has potential to use the wrong window.

If the outerWindowId is passed to showPayment, we can iterate through the windows and use tabbrowser.getBrowserForOuterWindowID to find which browser (and thus which tabbrowser this came from).

[1] https://searchfox.org/mozilla-central/rev/8384a6519437f5eefbe522196f9ddf5c8b1d3fb4/browser/components/payments/paymentUIService.js#48
Whiteboard: [webpayments] [triage]
Priority: -- → P3
Whiteboard: [webpayments] [triage] → [webpayments-reserve]
Flags: qe-verify?
Priority: P3 → P2
Whiteboard: [webpayments-reserve] → [webpayments]
Flags: qe-verify? → qe-verify-
We currently have the tabId but the outerWindowID is more commonly used for associating UI to a tab. Eden, do you know the difference? We have helpers to find a tab/<browser> by outerWindowID but not tabId so I don't think it's commonly used.
Component: WebPayments UI → DOM: Web Payments
Flags: needinfo?(echuang)
Priority: P2 → P3
Product: Firefox → Core
Whiteboard: [webpayments] → [webpayments-reserve]
Does a tabId properly handle when a tab is moved to a new window?
I think tabId is different from outerWindowID. It should be changed to outerWindowID for UI.
Flags: needinfo?(echuang)
Priority: P3 → P1
Since outerWindowId is commonly used in front-end team, instead of passing tabId, passing outerWindowId to UI.
Assignee: nobody → echuang
Attachment #9021219 - Flags: review?(amarchesini)
Status: NEW → ASSIGNED
Comment on attachment 9021219 [details] [diff] [review]
Instead of passing tabId, passing outerWindowId to Payment UI component.

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

lgtm

::: dom/payments/PaymentRequestManager.cpp
@@ +464,5 @@
>    ConvertOptions(aOptions, options);
>  
> +  MOZ_ASSERT(aWindow->GetOuterWindow());
> +  uint64_t outerWindowId = aWindow->GetOuterWindow()->WindowID();
> +  

extra spaces

::: dom/payments/ipc/PaymentRequestParent.h
@@ +22,1 @@
>  

remove explicit
Attachment #9021219 - Flags: review?(amarchesini) → review+
Comment on attachment 9021219 [details] [diff] [review]
Instead of passing tabId, passing outerWindowId to Payment UI component.

You will need to update https://searchfox.org/mozilla-central/search?q=tabId&case=false&regexp=false&path=browser%2Fcomponents%2Fpayments too or tests will fail.

Replace `findBrowserByTabId` with:
```
  findBrowserByOuterWindowId(outerWindowId) {
    for (let win of BrowserWindowTracker.orderedWindows) {
      let browser = win.gBrowser.getBrowserForOuterWindowID(outerWindowId);
      if (!browser) {
        continue;
      }
      return browser;
    }

    this.log.error("findBrowserByOuterWindowId: No browser found for outerWindowId:",
                   outerWindowId);
    return null;
  },
```

In the other cases tabId can simply be renamed to `outerWindowId`, don't worry about the values 9/null as they are just mock/default values.
Attachment #9021219 - Flags: feedback-
Hello Matt 
Thanks for your feedback, I have a part 2 patch modifies the part you mentioned.
And I would like to ask your review, do you use Phabricator for reviewing process? or just like the attached part 1? 

(In reply to Matthew N. [:MattN] (PM me if requests are blocking you) from comment #7)
> Comment on attachment 9021219 [details] [diff] [review]
> Instead of passing tabId, passing outerWindowId to Payment UI component.
> 
> You will need to update
> https://searchfox.org/mozilla-central/
> search?q=tabId&case=false&regexp=false&path=browser%2Fcomponents%2Fpayments
> too or tests will fail.
> 
> Replace `findBrowserByTabId` with:
> ```
>   findBrowserByOuterWindowId(outerWindowId) {
>     for (let win of BrowserWindowTracker.orderedWindows) {
>       let browser = win.gBrowser.getBrowserForOuterWindowID(outerWindowId);
>       if (!browser) {
>         continue;
>       }
>       return browser;
>     }
> 
>     this.log.error("findBrowserByOuterWindowId: No browser found for
> outerWindowId:",
>                    outerWindowId);
>     return null;
>   },
> ```
> 
> In the other cases tabId can simply be renamed to `outerWindowId`, don't
> worry about the values 9/null as they are just mock/default values.
Flags: needinfo?(MattN+bmo)
(In reply to Eden Chuang[:edenchuang] from comment #8)
> Hello Matt 
> Thanks for your feedback, I have a part 2 patch modifies the part you
> mentioned.
> And I would like to ask your review, do you use Phabricator for reviewing
> process? or just like the attached part 1? 

I prefer Phabricator but either way is fine. Thanks
Flags: needinfo?(MattN+bmo)
The part 1 patch supports outerWindowId be saved in nsIPaymentRequest, such that
Payment UI component can get the browser by outerWindowId which is an intuitive
way. The part 2 patch supports Payment UI get the browser by outerWindowId
Hello Matt

I have tried the feedback in comment 8, but it crashes the dom/payments/test/test_abortPayment.html with following error message

JavaScript error: file:///Users/echuang/Firefox/gecko/obj-x86_64-apple-darwin17.7.0/dist/NightlyDebug.app/Contents/Resources/browser/components/paymentUIService.js, line 54: TypeError: merchantBrowser is null; can't access its "ownerGlobal" property

Instead of using comment 8, the way in bug description for getting the chromeWindow is used. 
let chromeWindow = Services.wm.getMostRecentWindow("navigator:browser");

But it might not a good idea to use getMostRecentWindow().
Flags: needinfo?(MattN+bmo)
Sorry, I reviewed the patch before seeing this…

I may have made a mistake in my untested code but the approach and APIs I gave are the right ones. It's also possible that outerWindowId is only set later than the tabId on a <browser>?

It's also possible there is a race exposed by test_abortPayment.html as it may be trying to abort the payment and close the tab before showPayment has run? Just a guess without looking at the test.
Flags: needinfo?(MattN+bmo)
The root cause of the crash is tabbrowser._outerWindowIDBrowserMap only saved the top outerWindowId. So in order to get the correct browser object in paymentUIService.js, I modify the part 1 patch to passing the top outerWindowId to UI component.
Comment on attachment 9022731 [details] [diff] [review]
Part 1 - Instead of passing tabId, passing outerWindowId to Payment UI component.

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

::: dom/interfaces/payments/nsIPaymentRequest.idl
@@ +81,4 @@
>  [scriptable, builtinclass, uuid(2fa36783-d684-4487-b7a8-9def6ae3128f)]
>  interface nsIPaymentRequest : nsISupports
>  {
> +  readonly attribute uint64_t outerWindowId;

Please add a comment indicating this is for the top-level window/frame. Maybe it should be renamed to make that clearer too?
Keywords: checkin-needed
Pushed by rgurzau@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6bd92f118af5
Part 2 - Get the marchentBrowser by outerWindowId in PaymentUIService.js. r=MattN
https://hg.mozilla.org/integration/mozilla-inbound/rev/1ab00b6e99fd
Part 1 - Instead of passing tabId, passing outerWindowId to Payment UI component. r=baku
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/6bd92f118af5
https://hg.mozilla.org/mozilla-central/rev/1ab00b6e99fd
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
Attachment #9021503 - Attachment is obsolete: false
Attachment #9021503 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.