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

RESOLVED FIXED in Firefox 65

Status

()

defect
P2
normal
RESOLVED FIXED
5 months ago
2 months ago

People

(Reporter: mccr8, Assigned: mccr8)

Tracking

(Blocks 1 bug, {memory-leak})

unspecified
mozilla66
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox-esr60 wontfix, firefox64 wontfix, firefox65 fixed, firefox66 fixed)

Details

(Whiteboard: [MemShrink:P2])

Attachments

(1 attachment)

(Assignee)

Description

5 months ago
The leak is a bunch of JS stuff. Maybe the XPCOM leak checker will report something more useful.
Priority: -- → P2
(Assignee)

Comment 1

4 months ago

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

(Assignee)

Comment 2

4 months ago

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.

(Assignee)

Comment 3

4 months ago

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
(Assignee)

Updated

4 months ago
Component: HTML: Form Submission → DOM: Web Storage

Updated

4 months ago
Whiteboard: [MemShrink] → [MemShrink:P2]
(Assignee)

Comment 4

4 months ago

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.

(Assignee)

Updated

4 months ago
Duplicate of this bug: 1518230
(Assignee)

Comment 6

4 months ago
nsVariant isn't cycle collected, so this can cause a leak.

Comment 7

4 months ago
Pushed by amccreight@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a3ca57d21a06
DataTransferItemList::Add should use nsVariantCC. r=nika

Comment 8

4 months ago
bugherder
Status: NEW → RESOLVED
Last Resolved: 4 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla66

Is this worth uplifting to Beta?

Flags: needinfo?(continuation)
(Assignee)

Comment 10

4 months ago

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.