874252 - Leak with expando on a DOMError associated with indexedDB request

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Jesse Ruderman]

Attachment: 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

Comment 1

[Masatoshi Kimura [:emk]]

IDBRequest::mError is not cycle-collected.
https://mxr.mozilla.org/mozilla-central/source/dom/indexedDB/IDBRequest.cpp?rev=1196b497640b#302

Comment 2

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(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 ...

Comment 3

[Jesse Ruderman]

"Checking" statically, dynamically, or manually?

Comment 4

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Manually would be a good place to start.

Comment 5

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

e.g. content/media/webspeech/recognition/SpeechRecognition.h appears to have a similar problem.  There are probably more.

Comment 6

[Andrea Marchesini [:baku]]

Attachment: patch

I don't see any problem in SpeechRecognition.

Comment 7

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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.

Comment 8

[Andrea Marchesini [:baku]]

I didn't add the test... should I?

Comment 9

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Yes.

Comment 10

[Andrea Marchesini [:baku]]

Attachment: patch

https://tbpl.mozilla.org/?tree=Try&rev=31be9d99b1e9

Comment 11

[Andrea Marchesini [:baku]]

I want to see a green result on try before asking to land.

Comment 12

[Boris Zbarsky [:bzbarsky]]

This is why we should be CCing all outgoing strong refs, period.  In my opinion.

And yes, please add the test.

Comment 13

[Andrea Marchesini [:baku]]

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

Comment 14

[Andrea Marchesini [:baku]]

Attachment: 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.

Comment 15

[Andrea Marchesini [:baku]]

Attachment: patch

test in a follow up

Comment 16

[(no longer active)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ed6e88a93ca0

Comment 17

[Phil Ringnalda (:philor)]

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

Comment 18

[Jesse Ruderman]

Did the test land?

Comment 19

[Andrea Marchesini [:baku]]

not yet. it's still in my queue of things to do.