1408234 - PaymentRequestService doesn't clear requests from closed documents

Status: RESOLVED FIXED

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

Description

[Matthew N. [:MattN]]

While working on payment UI tests, I noticed that PaymentRequestService's enumerator continues to hold payment requests after the document that created them was closed (if there was no response to showing the dialog).

If a page simply constructs a PaymentRequest without calling .show() then the service in the parent process will leak requests until shutdown.

The automated tests likely pave over the leak checking since they call `paymentSrv.cleanup()` all the time. We should definitely add a test that doesn't call cleanup in order to catch leaks like this. We should probably also consider replacing `cleanup` calls in other tests with `SpecialPowers.exactGC` in order to test the end-to-end cleanup.

Comment 1

[EdenChuang(ChenYu Chuang)]

Hello 

I am Eden, back to continue this bug I worked.

Comment 2

[Henri Sivonen (:hsivonen)]

Eden, do you have an estimate on when you'd be able to fix this bug?

What does the expected fix look like? Should the inner window maintain a list of the IDs of the PaymentRequests created in its context and call PaymentRequestManager::AbortPayment() for each one upon the destruction of the inner window?

Does calling AbortPayment() with IDs of payments that don't exist or that have succeeded previously have any adverse effects?

Comment 3

[EdenChuang(ChenYu Chuang)]

(In reply to Henri Sivonen (:hsivonen) from comment #2)
> Eden, do you have an estimate on when you'd be able to fix this bug?
> 
> What does the expected fix look like? Should the inner window maintain a
> list of the IDs of the PaymentRequests created in its context and call
> PaymentRequestManager::AbortPayment() for each one upon the destruction of
> the inner window?
> 

I will fix this in these two days.
No, the PaymentRequest IDs and document association would be maintained in PaymentRequestManager.
While Document::Destory() is called, PaymentRequestManager.cleanup will be called to remove PaymentRequests associated with the document.

> Does calling AbortPayment() with IDs of payments that don't exist or that
> have succeeded previously have any adverse effects?

I think no any adverse effects.

Comment 4

[EdenChuang(ChenYu Chuang)]

Attachment: Bug1408234 - Cleanup the PaymentRequests when document close. r?baku

Let PaymentRequest inherit from nsIDocumentActivity interface.               

Calling RegisterActivityObserver() and UnregisterActivityObserver() in constructor and destructor to get activity notifications from document.      

When receiving the notification, NotifyOwnerDocumentActivityChanged() will check the owner document's activity status. If the status is disabled, calling PaymentRequestManager::CleanupPayment() to clean the PaymentRequest in content process and also sending the clean information to chrome process.

Comment 5

[EdenChuang(ChenYu Chuang)]

Attachment: Bug1408234 - mochitest for PaymentRequest cleanup after document closed. r?baku

1. add a new mochitest to test cleanup function by changing the attribute of iframe.src.                                                               
2. remove unnecessary PaymentRequestService.cleanup() call in the test suite.

Comment 6

[EdenChuang(ChenYu Chuang)]

try server result

https://treeherder.mozilla.org/#/jobs?repo=try&revision=627393e8febd076dc108d3369ddd9ff8c137d7d2

Comment 7

[Andrea Marchesini [:baku]]

Comment on attachment 8982120 [details] [diff] [review]
Bug1408234 - Cleanup the PaymentRequests when document close. r?baku

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

IsActive() returns false if the page is not focused. Are we sure this is the behavior we want?

Comment 8

[EdenChuang(ChenYu Chuang)]

(In reply to Andrea Marchesini [:baku] from comment #7)
> Comment on attachment 8982120 [details] [diff] [review]
> Bug1408234 - Cleanup the PaymentRequests when document close. r?baku
> 
> Review of attachment 8982120 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> IsActive() returns false if the page is not focused. Are we sure this is the
> behavior we want?

Maybe I missed something. I am a little bit confused why IsActive() returns false if the page is not focused?

I had tried by switching to another tab or another window and minimize the window which the page creates a PaymentRequest.
But I all get true from nsIDocument::IsActive().

According to the definition of nsIDocument::IsActive()
https://searchfox.org/mozilla-central/source/dom/base/nsIDocument.h#2522 
nsIDocument::IsActive() returns false when the mDocumentContainer is null or mRemovedFromDocShell is false.
However, removing the focus or hiding the page would not set mDocumentContainer as null or set mRemovedFromDocShell as false.

Did I misunderstand something?

Comment 9

[Marion Daly [:mdaly]]

P1, has an open need info for 20 days. Andrea, can you respond when you have time?

Comment 10

[Andrew Overholt [:overholt]]

So this isn't ready to land, Eden?

Comment 11

[EdenChuang(ChenYu Chuang)]

Because the fix of bug 1442453, the patches need to be updated. Will update the patches in these two days.

Comment 12

[Jean Gong :jgong]

Eden, Do you have an update on the patch for this bug?

Comment 13

[Eden Chuang[:edenchuang]]

Attachment: Bug1408234 - Cleanup the PaymentRequests when document close. r=baku

Update the patch according to the code base change.

Comment 14

[Eden Chuang[:edenchuang]]

Attachment: Bug1408234 - mochitest for PaymentRequest cleanup after document closed. r=baku

Update the patch according to the code base change.

Comment 15

[Eden Chuang[:edenchuang]]

Try server result.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3022906a206f3bb781ee8b8dc2837cf4bdb58cd7

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c7536967b2f9
Cleanup the PaymentRequests when document close. r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/19f9ed0f53b9
mochitest for PaymentRequest cleanup after document closed. r=baku

Comment 17

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/c7536967b2f9
https://hg.mozilla.org/mozilla-central/rev/19f9ed0f53b9