PaymentRequestService doesn't clear requests from closed documents

RESOLVED FIXED in Firefox 63

Status

()

P1
normal
RESOLVED FIXED
a year ago
4 months ago

People

(Reporter: MattN, Assigned: edenchuang)

Tracking

({memory-leak})

Trunk
mozilla63
memory-leak
Points:
---
Bug Flags:
in-testsuite +
qe-verify -

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox-esr60 wontfix, firefox58 wontfix, firefox61 wontfix, firefox62 wontfix, firefox63 fixed)

Details

(Whiteboard: [webpayments] [MemShrink:P2])

Attachments

(2 attachments, 2 obsolete attachments)

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)

Updated

a year ago
Assignee: nobody → echuang
Status: NEW → ASSIGNED
(Assignee)

Updated

a year ago
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)
Created attachment 8982120 [details] [diff] [review]
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.
Attachment #8982120 - Flags: review?(amarchesini)
Created attachment 8982121 [details] [diff] [review]
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.
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+
Keywords: checkin-needed
Keywords: checkin-needed
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)

Comment 12

5 months ago
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-
(Assignee)

Comment 13

4 months ago
Created attachment 8999579 [details] [diff] [review]
Bug1408234 - Cleanup the PaymentRequests when document close. r=baku

Update the patch according to the code base change.
Attachment #8982120 - Attachment is obsolete: true
Flags: needinfo?(chenyu.chuang)
Attachment #8999579 - Flags: review+
(Assignee)

Comment 14

4 months ago
Created attachment 8999580 [details] [diff] [review]
Bug1408234 - mochitest for PaymentRequest cleanup after document closed. r=baku

Update the patch according to the code base change.
Attachment #8982121 - Attachment is obsolete: true
Attachment #8999580 - Flags: review+
(Assignee)

Updated

4 months ago
Keywords: checkin-needed

Comment 16

4 months ago
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

Comment 17

4 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/c7536967b2f9
https://hg.mozilla.org/mozilla-central/rev/19f9ed0f53b9
Status: ASSIGNED → RESOLVED
Last Resolved: 4 months ago
status-firefox63: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
status-firefox58: affected → wontfix
status-firefox61: --- → wontfix
status-firefox62: --- → wontfix
status-firefox-esr52: --- → unaffected
status-firefox-esr60: --- → wontfix
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.