The default bug view has changed. See this FAQ.

Leak with expando on a DOMError associated with indexedDB request

RESOLVED FIXED in mozilla24

Status

()

Core
DOM
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: Jesse Ruderman, Assigned: baku)

Tracking

(Blocks: 1 bug, {mlk, regression, testcase})

Trunk
mozilla24
x86_64
Mac OS X
mlk, regression, testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [MemShrink])

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

4 years ago
Created attachment 751879 [details]
testcase

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]
IDBRequest::mError is not cycle-collected.
https://mxr.mozilla.org/mozilla-central/source/dom/indexedDB/IDBRequest.cpp?rev=1196b497640b#302
(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

4 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

4 years ago
Created attachment 752137 [details] [diff] [review]
patch

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

4 years ago
I didn't add the test... should I?
Keywords: checkin-needed
Yes.
(Assignee)

Comment 10

4 years ago
Created attachment 752173 [details] [diff] [review]
patch

https://tbpl.mozilla.org/?tree=Try&rev=31be9d99b1e9
Attachment #752137 - Attachment is obsolete: true
Attachment #752173 - Flags: review+
(Assignee)

Comment 11

4 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

4 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

4 years ago
Assignee: nobody → amarchesini
(Assignee)

Comment 14

4 years ago
Created attachment 753756 [details] [diff] [review]
patch

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

4 years ago
Created attachment 754457 [details] [diff] [review]
patch

test in a follow up
Attachment #753756 - Attachment is obsolete: true
Attachment #754457 - Flags: review+
(Assignee)

Updated

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

Comment 18

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

Comment 19

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