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

Status: RESOLVED FIXED

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

Description

[Andrew McCreight [:mccr8]]

The leak is a bunch of JS stuff. Maybe the XPCOM leak checker will report something more useful.

Comment 1

[Andrew McCreight [:mccr8]]

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

Comment 2

[Andrew McCreight [:mccr8]]

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.

Comment 3

[Andrew McCreight [:mccr8]]

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

Comment 4

[Andrew McCreight [:mccr8]]

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.

Comment 6

[Andrew McCreight [:mccr8]]

Attachment: Bug 1517577 - DataTransferItemList::Add should use nsVariantCC.

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

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a3ca57d21a06
DataTransferItemList::Add should use nsVariantCC. r=nika

Comment 8

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/a3ca57d21a06

Comment 9

[Ryan VanderMeulen [:RyanVM]]

Is this worth uplifting to Beta?

Comment 10

[Andrew McCreight [:mccr8]]

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

Comment 11

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/75bae5cabb1c