1646556 - Fix GetInProcessTop/GetInProcessParentDocument usage in PaymentRequest code

Status: RESOLVED FIXED

(Core :: DOM: Web Payments, defect, P3)

Description

[Kris Maglione [:kmag]]

These methods are not Fission-compatible, and will do the wrong thing for windows with OOP ancestors.

Comment 1

[Jens Stutte [:jstutte]]

Should this inherit Fission M6a from bug 1642433 ?

Comment 2

[Kris Maglione [:kmag]]

(In reply to Jens Stutte [:jstutte] from comment #1)

Should this inherit Fission M6a from bug 1642433 ?

No, we'll assign a priority during this week's triage.

Comment 3

[Nika Layzell [:nika] (ni? for response)]

This code path should be possible to implement by walking the BrowsingContext tree, and checking if each WindowContext is currently active within it's BrowsingContext. https://searchfox.org/mozilla-central/rev/c6676771df58c6e0098574bc6b11517acbf264cf/dom/payments/PaymentRequest.cpp#1169-1177

This call-site would probably be improved by using the top browsing context ID rather than outer window ID: https://searchfox.org/mozilla-central/rev/c6676771df58c6e0098574bc6b11517acbf264cf/dom/payments/PaymentRequestManager.cpp#502-508. That being said, the top outer window ID is also avaliable with BrowsingContext::GetTopWindowContext() and WindowContext::GetOuterWindowId(). (though the only call site looks like it'd work with BrowsingContext ID just fine: https://searchfox.org/mozilla-central/rev/c6676771df58c6e0098574bc6b11517acbf264cf/browser/components/payments/PaymentUIService.jsm#61-63)

(didn't look super hard, so there may be other callers)

Comment 4

[Neha Kochar [:neha]]

Eden, can you fix this for Fission please?

Comment 5

[Eden Chuang[:edenchuang]]

Attachment: Bug 1646556 - Make PaymentRequest APIs be fission compatible.

Comment 6

[BugBot [:suhaib / :marco/ :calixte]]

There's a r+ patch which didn't land and no activity in this bug for 2 weeks.
:edenchuang, could you have a look please?
For more information, please visit auto_nag documentation.

Comment 7

[Karl Tomlinson (:karlt)]

We now have nsPIDOMWindowInner::IsFullyActive(), which adds required IsDiscarded() tests on the BrowsingContext. It also has IsDiscarded() tests on the WindowContext, but I suspect that may not be necessary, right now at least.

However, simply using that is not sufficient in PaymentRequest::NotifyOwnerDocumentActivityChanged(), as is detected by test_closePayment.html.
Analysis of testCloseByRemovingIframe() identifies that the last NotifyOwnerDocumentActivityChanged() call is triggered from nsDocShell::Destroy() before mScriptGlobal->DetachFromDocShell().
Essentially the document is becoming not fully active, but is not there yet.
The current doc tests happen to catch that case, but I suspect they may not still do so with activity changes in frames across process boundaries.

There's no plan to enable PaymentRequest in the near future, so this is not a priority.

Comment 8

[Chris Peterson [:cpeterson]]

(In reply to Karl Tomlinson (back Oct 5 :karlt) from comment #7)

There's no plan to enable PaymentRequest in the near future, so this is not a priority.

In that case, this bug should continue to block PaymentRequest (meta bug 1318984) but doesn't need to block shipping Fission MVP.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3237b019d8c4
Make PaymentRequest APIs be fission compatible. r=nika

Comment 11

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/3237b019d8c4