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.

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

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?

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

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.

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.

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

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?

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

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

So this isn't ready to land, Eden?

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

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

Update the patch according to the code base change.

Update the patch according to the code base change.

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

Pushed by apavel@mozilla.com: 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

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