Closed Bug 1517577 Opened 6 years ago Closed 6 years ago

Leaks in WPT tests in /html/semantics/forms/form-submission-0/

Categories

(Core :: Storage: localStorage & sessionStorage, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla66
Tracking Status
firefox-esr60 --- wontfix
firefox64 --- wontfix
firefox65 --- fixed
firefox66 --- fixed

People

(Reporter: mccr8, Assigned: mccr8)

References

(Blocks 1 open bug)

Details

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

Attachments

(1 file)

The leak is a bunch of JS stuff. Maybe the XPCOM leak checker will report something more useful.
Priority: -- → P2

In the XPCOM leak checker, this directory leaks 3 windows and 3 documents, among other things.

I get two window/documents leaks with this test:

./mach wpt html/semantics/forms/form-submission-0/submit-file.sub.html

I didn't find another test that leaks, but maybe there is one. That's at least a start.

There's a Blob that is leaking things. I'll see if I can figure out what is holding that alive tomorrow.

Assignee: nobody → continuation
Component: HTML: Form Submission → DOM: Web Storage
Whiteboard: [MemShrink] → [MemShrink:P2]

I was able to use DMD heap scan mode to figure out what was going wrong. DataTransferItemList::Add needs to use nsVariantCC instead of nsVariant. I should probably make that less of a footgun somehow.

nsVariant isn't cycle collected, so this can cause a leak.
Pushed by amccreight@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/a3ca57d21a06 DataTransferItemList::Add should use nsVariantCC. r=nika
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla66

Is this worth uplifting to Beta?

Flags: needinfo?(continuation)

Comment on attachment 9035496 [details]
Bug 1517577 - DataTransferItemList::Add should use nsVariantCC.

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: Bug 906420

User impact if declined: Bad leaks, but it isn't clear if anybody is actually hitting this in the wild. This is an old bug.

Is this code covered by automated tests?: Yes

Has the fix been verified in Nightly?: Yes

Needs manual test from QE?: No

If yes, steps to reproduce:

List of other uplifts needed: None

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): This just makes an object be cycle collected instead of not cycle collected.

String changes made/needed: none

Flags: needinfo?(continuation)
Attachment #9035496 - Flags: approval-mozilla-beta?

Comment on attachment 9035496 [details]
Bug 1517577 - DataTransferItemList::Add should use nsVariantCC.

[Triage Comment]
Low risk fix for a possibly-bad leak. Approved for 65.0b11.

Attachment #9035496 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: