Leak with expando on a DOMError associated with indexedDB request

RESOLVED FIXED in mozilla24

Status

()

defect
RESOLVED FIXED
6 years ago
2 months ago

People

(Reporter: jruderman, Assigned: baku)

Tracking

(Blocks 1 bug, {memory-leak, regression, testcase})

Trunk
mozilla24
x86_64
macOS
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [MemShrink])

Attachments

(1 attachment, 4 obsolete attachments)

Reporter

Description

6 years ago
Posted file testcase (obsolete) —
With
  user_pref("dom.indexedDB.enabled", false);

The testcase causes Firefox to leak nsGlobalWindow and nsDocument objects.  (You can see the leaks by running a debug build with XPCOM_MEM_LEAK_LOG=2).

Based on when my DOM fuzzer started reporting this, I'm guessing it's a regression from:

changeset:   http://hg.mozilla.org/mozilla-central/rev/1196b497640b
user:        Andrea Marchesini
date:        Sat May 18 13:52:06 2013 -0400
summary:     Bug 870856 - Convert DOMError to WebIDL. r=Ms2ger, r=bz
Blocks: 870856
Whiteboard: [MemShrink]
(In reply to Masatoshi Kimura [:emk] from comment #1)
> IDBRequest::mError is not cycle-collected.
> https://mxr.mozilla.org/mozilla-central/source/dom/indexedDB/IDBRequest.
> cpp?rev=1196b497640b#302

Yeah, that didn't use to matter, but now it does.

Worth checking other DOM objects too ...
Reporter

Comment 3

6 years ago
"Checking" statically, dynamically, or manually?
Manually would be a good place to start.
e.g. content/media/webspeech/recognition/SpeechRecognition.h appears to have a similar problem.  There are probably more.
Assignee

Comment 6

6 years ago
Posted patch patch (obsolete) — Splinter Review
I don't see any problem in SpeechRecognition.
Attachment #752137 - Flags: review?(khuey)
Comment on attachment 752137 [details] [diff] [review]
patch

Review of attachment 752137 [details] [diff] [review]:
-----------------------------------------------------------------

Ah I misread that file.  I didn't realize that SpeechEvent is a separate class.
Attachment #752137 - Flags: review?(khuey) → review+
Assignee

Comment 8

6 years ago
I didn't add the test... should I?
Keywords: checkin-needed
Assignee

Comment 10

6 years ago
Posted patch patch (obsolete) — Splinter Review
https://tbpl.mozilla.org/?tree=Try&rev=31be9d99b1e9
Attachment #752137 - Attachment is obsolete: true
Attachment #752173 - Flags: review+
Assignee

Comment 11

6 years ago
I want to see a green result on try before asking to land.
Keywords: checkin-needed
This is why we should be CCing all outgoing strong refs, period.  In my opinion.

And yes, please add the test.
Assignee

Comment 13

6 years ago
Yep, tests are included in the patch.
and the first try request was contains too many unrelated patches.

https://tbpl.mozilla.org/?tree=Try&rev=8747406aefe8
Assignee

Updated

6 years ago
Assignee: nobody → amarchesini
Assignee

Comment 14

6 years ago
Posted patch patch (obsolete) — Splinter Review
Patch is ready but I still having problem with test_ipc.html. It's not able to catch an exception generated by indexedDB.deleteDatabase().

Test_ipc.html runs all the devicestorage mochitests but it fails with test_bug874252.html. Any hint? Of course test_bug874252.html is green when it runs out of IPC.

The exception is InvalidStateError, and it's generated because in order to test this bug I disabled devicestorage via prefs.
Attachment #751879 - Attachment is obsolete: true
Attachment #752173 - Attachment is obsolete: true
Attachment #753756 - Flags: review+
Assignee

Comment 15

6 years ago
Posted patch patchSplinter Review
test in a follow up
Attachment #753756 - Attachment is obsolete: true
Attachment #754457 - Flags: review+
Assignee

Updated

6 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/ed6e88a93ca0
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
Reporter

Comment 18

6 years ago
Did the test land?
Flags: needinfo?(amarchesini)
Flags: in-testsuite?
Assignee

Comment 19

6 years ago
not yet. it's still in my queue of things to do.
Flags: needinfo?(amarchesini)
Component: DOM → DOM: Core & HTML
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.