Closed Bug 1408234 Opened 7 years ago Closed 6 years ago

PaymentRequestService doesn't clear requests from closed documents

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox-esr52 --- unaffected
firefox-esr60 --- wontfix
firefox58 --- wontfix
firefox61 --- wontfix
firefox62 --- wontfix
firefox63 --- fixed

People

(Reporter: MattN, Assigned: edenchuang)

Details

(Keywords: memory-leak, Whiteboard: [webpayments] [MemShrink:P2])

Attachments

(2 files, 2 obsolete files)

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.
Assignee: nobody → echuang
Status: NEW → ASSIGNED
Priority: -- → P2
Whiteboard: [memshrink] → [MemShrink:P2]
Assignee: echuang → chenyu.chuang
Priority: P2 → P1
Hello 

I am Eden, back to continue this bug I worked.
Whiteboard: [MemShrink:P2] → [webpayments] [MemShrink:P2]
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?
Flags: needinfo?(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.
Flags: needinfo?(chenyu.chuang)
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.
Attachment #8982120 - Flags: review?(amarchesini)
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.
Attachment #8982121 - Flags: review?(amarchesini)
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?
Attachment #8982120 - Flags: review?(amarchesini) → review-
Attachment #8982121 - Flags: review?(amarchesini) → review+
(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?
Flags: needinfo?(amarchesini)
P1, has an open need info for 20 days. Andrea, can you respond when you have time?
Attachment #8982120 - Flags: review- → review+
So this isn't ready to land, Eden?
Flags: needinfo?(amarchesini) → needinfo?(chenyu.chuang)
Because the fix of bug 1442453, the patches need to be updated. Will update the patches in these two days.
Flags: needinfo?(chenyu.chuang)
Eden, Do you have an update on the patch for this bug?
Flags: needinfo?(chenyu.chuang)
Whiteboard: [webpayments] [MemShrink:P2] → [webpayments-reserve] [MemShrink:P2]
Flags: qe-verify?
Whiteboard: [webpayments-reserve] [MemShrink:P2] → [webpayments] [MemShrink:P2]
Assignee: chenyu.chuang → echuang
Flags: qe-verify? → qe-verify-
Update the patch according to the code base change.
Attachment #8982120 - Attachment is obsolete: true
Flags: needinfo?(chenyu.chuang)
Attachment #8999579 - Flags: review+
Update the patch according to the code base change.
Attachment #8982121 - Attachment is obsolete: true
Attachment #8999580 - Flags: review+
Keywords: checkin-needed
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
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/c7536967b2f9
https://hg.mozilla.org/mozilla-central/rev/19f9ed0f53b9
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: