840260 - AudioChannelManager remnant in nsDOMClassInfoClasses.h

Status: RESOLVED FIXED

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

Description

[Benoit Jacob [:bjacob] (mostly away)]

B2G + mozilla-central debug build asserts here with the assertion at nsDOMClassInfo.cpp:3647,

      MOZ_NOT_REACHED("The number of items in sClassInfoData doesn't match the "
                      "number of nsIDOMClassInfo ID's, this is bad! Fix it!");

To debug this, I dumped sClassInfoData and the stuff in nsDOMClassInfoClasses (by redefining DOMCI_CLASS) and get multiple differences. The dumps are attached to this bug. The diff is:

--- DOMClassInfoClasses 2013-02-11 16:44:07.121159202 -0500
+++ sClassInfoData      2013-02-11 16:44:11.101159297 -0500
@@ -1,9 +1,9 @@
+
 AnimationEvent
 ArchiveReader
 ArchiveRequest
 AsyncScrollEventDetail
 Attr
-AudioChannelManager
 BarProp
 BeforeUnloadEvent
 Blob
@@ -31,7 +31,6 @@
 Comment
 CompositionEvent
 ContentFrameMessageManager
-ContentList
 CRMFObject
 Crypto
 CSSCharsetRule
@@ -68,7 +67,6 @@
 DOMConstructor
 DOMError
 DOMException
-DOMFileHandle
 DOMPrototype
 DOMRequest
 DOMStringList
@@ -78,6 +76,7 @@
 Event
 EventListenerInfo
 File
+FileHandle
 FileList
 FileReader
 FileRequest
@@ -159,7 +158,6 @@
 IDBCursorWithValue
 IDBDatabase
 IDBFactory
-IDBFileHandle
 IDBIndex
 IDBKeyRange
 IDBObjectStore
@@ -172,7 +170,6 @@
 LocalMediaStream
 Location
 LockedFile
-MathMLElement
 MediaError
 MediaList
 MediaQueryList
@@ -247,6 +244,7 @@
 SVGAnimatedRect
 SVGAnimatedString
 SVGDocument
+SVGElement
 SVGEvent
 SVGFEBlendElement
 SVGFEColorMatrixElement
@@ -281,7 +279,6 @@
 SVGRect
 SVGStringList
 SVGTitleElement
-SVGUnknownElement
 SVGZoomEvent
 Telephony
 TelephonyCall

Comment 1

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: DOMClassInfoClasses dump

Comment 2

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: sClassInfoData

Comment 3

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

We should make these crash opt builds if they don't already.

Comment 4

[Benoit Jacob [:bjacob] (mostly away)]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #3)
> We should make these crash opt builds if they don't already.

They don't already due to the #ifdef DEBUG at nsDOMClassInfo.cpp:3642.

FWIW here is the code used to dump this (at that same location):

#ifdef DEBUG
  {
    uint32_t i = ArrayLength(sClassInfoData);

    if (i != eDOMClassInfoIDCount) {
      for (uint32_t x = 0; x < i; x++) {
        printf_stderr("sClassInfoData %s\n", sClassInfoData[x].mName);
      }

#define DOMCI_CLASS(X) printf_stderr("domciclass %s\n", #X);
#include "nsDOMClassInfoClasses.h"
#undef DOMCI_CLASS

      MOZ_NOT_REACHED("The number of items in sClassInfoData doesn't match the "
                      "number of nsIDOMClassInfo ID's, this is bad! Fix it!");

      return NS_ERROR_NOT_INITIALIZED;
    }

Comment 5

[Benoit Jacob [:bjacob] (mostly away)]

So, most of the differences were just my dumping being fooled by special cases. The only real difference is that AudioChannelManager is not listed on the sClassInfoData side.

Comment 6

[Benoit Jacob [:bjacob] (mostly away)]

Looks like this regressed in bug 838172.

Comment 7

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: remove stuff from classinfoclasses --- fix B2G debug builds

This fixes the assertion reported above.

Comment 8

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: assert even in non-DEBUG builds

This implemented Kyle's suggestion in comment 3. I just only applied this to the first assertion here (which was hit above), not to the for loops below, so that we can at least land that without worrying about performance.

We have 320 classes here on B2G, two for loops, so if we enabled them in release builds, that would cost us 640 (cache friendly) memory derefs + comparisons. I have no idea what the perf requirements on this code are.

Comment 9

[Masatoshi Kimura [:emk]]

(In reply to Benoit Jacob [:bjacob] from comment #8)
> Created attachment 712654 [details] [diff] [review]
> assert even in non-DEBUG builds
> 
> This implemented Kyle's suggestion in comment 3. I just only applied this to
> the first assertion here (which was hit above), not to the for loops below,
> so that we can at least land that without worrying about performance.

Is it impossible to make the first assertion static assert?

Comment 11

[Benoit Jacob [:bjacob] (mostly away)]

(In reply to Masatoshi Kimura [:emk] from comment #9)
> Is it impossible to make the first assertion static assert?

It is very much possible as it's comparing the size of a static array to an enum value; patch coming.

Comment 12

[Benoit Jacob [:bjacob] (mostly away)]

build fix landed without review:
https://hg.mozilla.org/integration/mozilla-inbound/rev/06090cb1dc1f

Comment 13

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: make it a static assert

also some code simplification around there, removed a now-useless {...} scope, and use size_t instead of uint32_t for a generic array index variable.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/06090cb1dc1f

Comment 15

[Masatoshi Kimura [:emk]]

Should have been tagged with [leave open]

Comment 16

[Benoit Jacob [:bjacob] (mostly away)]

As you wish, but FWIW I intentionally didn't --- the first patch here fixes the bug at hand, the second one is just a follow-up to make sure that this doesn't happen again.

Comment 17

[Masatoshi Kimura [:emk]]

Then I think the second patch should have been a separate bug.

Comment 19

[Andrew Halberstadt [:ahal]]

We're running debug tests on the cedar branch and we noticed this assertion. The patch fixed it but now we just hit a new assertion:
F/MOZ_Assert(  642): Assertion failure: size_t(data) % sizeof(Value) == 0, at /builds/slave/ced-ics_a7_g-d-000000000000000/build/js/src/jsscript.cpp:1751

Could this be related? Or do you think this is a separate issue?
Full log: https://tbpl.mozilla.org/php/getParsedLog.php?id=19742043&tree=Cedar&full=1

Comment 20

[Benoit Jacob [:bjacob] (mostly away)]

This doesn't seem related, from just this log.

Comment 21

[Benoit Jacob [:bjacob] (mostly away)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/e82104805907

Comment 22

[Ed Morley [:emorley]]

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