Closed Bug 469004 Opened 11 years ago Closed 11 years ago

RECURSION_LEVEL assertions in nsHTMLEntities due to races from speculative parsing thread

Categories

(Core :: DOM: HTML Parser, defect, P3)

defect

Tracking

()

RESOLVED FIXED
mozilla1.9.2a1

People

(Reporter: dbaron, Assigned: dbaron)

Details

(Keywords: assertion)

Attachments

(1 file, 1 obsolete file)

Lately, when I've been running a build for a few days, at some point (after quite a while of running it) it gets into a state where I see huge numbers of these assertions:

###!!! ASSERTION: RECURSION_LEVEL(table_) > 0: 'RECURSION_LEVEL(table) > 0', file pldhash.c, line 691

At stacks like these:

nsHTMLEntities::EntityToUnicode(nsCString const&) (/home/dbaron/builds/mozilla-central/mozilla/parser/htmlparser/src/nsHTMLEntities.cpp:201)
nsHTMLEntities::EntityToUnicode(nsAString_internal const&) (/home/dbaron/builds/mozilla-central/mozilla/parser/htmlparser/src/nsHTMLEntities.cpp:215)
ConsumeEntity (/home/dbaron/builds/mozilla-central/mozilla/parser/htmlparser/src/nsHTMLTokens.cpp:96)
ConsumeUntil (/home/dbaron/builds/mozilla-central/mozilla/parser/htmlparser/src/nsHTMLTokens.cpp:168)
ConsumeQuotedString (/home/dbaron/builds/mozilla-central/mozilla/parser/htmlparser/src/nsHTMLTokens.cpp:1683)
CAttributeToken::Consume(unsigned short, nsScanner&, int) (/home/dbaron/builds/mozilla-central/mozilla/parser/htmlparser/src/nsHTMLTokens.cpp:1814)
nsHTMLTokenizer::ConsumeAttributes(unsigned short, CToken*, nsScanner&) (/home/dbaron/builds/mozilla-central/mozilla/parser/htmlparser/src/nsHTMLTokenizer.c
nsHTMLTokenizer::ConsumeStartTag(unsigned short, CToken*&, nsScanner&, int&) (/home/dbaron/builds/mozilla-central/mozilla/parser/htmlparser/src/nsHTMLTokeni
nsHTMLTokenizer::ConsumeTag(unsigned short, CToken*&, nsScanner&, int&) (/home/dbaron/builds/mozilla-central/mozilla/parser/htmlparser/src/nsHTMLTokenizer.c
nsHTMLTokenizer::ConsumeToken(nsScanner&, int&) (/home/dbaron/builds/mozilla-central/mozilla/parser/htmlparser/src/nsHTMLTokenizer.cpp:503)
nsSpeculativeScriptThread::Run() (/home/dbaron/builds/mozilla-central/mozilla/parser/htmlparser/src/nsParser.cpp:384)
nsThreadPool::Run() (/home/dbaron/builds/mozilla-central/mozilla/xpcom/threads/nsThreadPool.cpp:220)
nsThread::ProcessNextEvent(int, int*) (/home/dbaron/builds/mozilla-central/mozilla/xpcom/threads/nsThread.cpp:525)
NS_ProcessNextEvent_P(nsIThread*, int) (/home/dbaron/builds/mozilla-central/obj/firefox-debugopt/xpcom/build/nsThreadUtils.cpp:227)
nsThread::ThreadFunc(void*) (/home/dbaron/builds/mozilla-central/mozilla/xpcom/threads/nsThread.cpp:253)
_pt_root (/home/dbaron/builds/mozilla-central/mozilla/nsprpub/pr/src/pthreads/ptthread.c:224)
start_thread (pthread_create.c:0)
__clone (/lib/libc.so.6)


It seems like nsHTMLEntities is using a PLDHashTable on multiple threads without any locking.  It's possible this may well be safe (although I'm not sure; we should double-check).

If this is safe, we could probably make the assertions much rarer by using PR_AtomicIncrement/Decrement, since we wouldn't get into a bad state, although that wouldn't make the assertions disappear completely.
As an alternative to atomic ops, we could add a (DEBUG-only?) function to mark a jsdhash/pldhash as constant (which would set the recursion level to something magic, like -1), and in that case:
 * assert that we never mutate the table again (except when destroying it)
 * weaken the recursion level assertions (since we know we'll never mutate the table) and stop mutating the recursion level
Keywords: assertion
Assignee: nobody → dbaron
OS: Linux → All
Hardware: x86 → All
Attached patch patch (obsolete) — Splinter Review
Note that I have a patch underneath this one in my patch queue to refresh pldhash.{h,c} from current jsdhash.{h,cpp}.  The changes in that one seem to be entirely minor reformatting.
Attachment #356241 - Flags: review?(brendan)
Attachment #356241 - Flags: review?(brendan) → review+
Comment on attachment 356241 [details] [diff] [review]
patch

>+/*
>+ * Most callers that assert about the recursion level don't care about
>+ * this magical value because they are asserting that mutation is
>+ * allowed (and therefore the level is 0 or 1, depending on whether they
>+ * incremented it).
>+ *
>+ * Only PL_DHashTableFinish needs to allow this special value.
>+ */
>+#define IMMUTABLE_RECURSION_LEVEL ((uint32)-1)
> 
> #define ENTRY_STORE_EXTRA                   sizeof(uint32)
>-#define INCREMENT_RECURSION_LEVEL(table_)   \
>-    JS_BEGIN_MACRO                          \
>-      ++RECURSION_LEVEL(table_);            \
>+#define INCREMENT_RECURSION_LEVEL(table_)                                     \
>+    JS_BEGIN_MACRO                                                            \
>+      if (RECURSION_LEVEL(table_) != IMMUTABLE_RECURSION_LEVEL) {             \
>+        ++RECURSION_LEVEL(table_);                                            \
>+      }                                                                       \
>     JS_END_MACRO
>-#define DECREMENT_RECURSION_LEVEL(table_)                  \
>-    JS_BEGIN_MACRO                                         \
>-      JSDHASH_ONELINE_ASSERT(RECURSION_LEVEL(table_) > 0); \
>-      --RECURSION_LEVEL(table_);                           \
>+#define DECREMENT_RECURSION_LEVEL(table_)                                     \
>+    JS_BEGIN_MACRO                                                            \
>+      if (RECURSION_LEVEL(table_) != IMMUTABLE_RECURSION_LEVEL) {             \
>+        JSDHASH_ONELINE_ASSERT(RECURSION_LEVEL(table_) > 0);                  \
>+        --RECURSION_LEVEL(table_);                                            \
>+      }                                                                       \
>     JS_END_MACRO

Nit: four space c-basic-offset applies in macros too. Also, prevailing style does not over-brace then or else clauses (only if any of condition, then or else is multiline).

>-    JS_ASSERT(RECURSION_LEVEL(table) == 0);
>+    JS_ASSERT(RECURSION_LEVEL(table) == 0 || RECURSION_LEVEL(table) == IMMUTABLE_RECURSION_LEVEL);

Even with new tw=99 sensibilities allowing column 80 transgression, this is a bit much -- split after the || ?

r=me with nits picked.

/be
(In reply to comment #3)
> >-    JS_ASSERT(RECURSION_LEVEL(table) == 0);
> >+    JS_ASSERT(RECURSION_LEVEL(table) == 0 || RECURSION_LEVEL(table) == IMMUTABLE_RECURSION_LEVEL);
> 
> Even with new tw=99 sensibilities allowing column 80 transgression, this is a
> bit much -- split after the || ?

That requires some serious plify_jsdhash.sed hacking, unfortunately, since it expects JS_ASSERT(...) to be on one line.

Probably the easiest workaround is to define a separate macro for RECURSION_LEVEL(table)==0 || RECURSION_LEVEL(table)==IMMUTABLE_RECURSION_LEVEL, although I'd tried to avoid that since it was a special case.
Attached patch patchSplinter Review
I think this addresses Brendan's comments; I added a new macro to avoid the overly long line.
Attachment #356241 - Attachment is obsolete: true
Attachment #356248 - Flags: review?(mrbkap)
Attachment #356241 - Flags: review?(mrbkap)
Attachment #356248 - Flags: review?(mrbkap) → review+
Comment on attachment 356248 [details] [diff] [review]
patch

Looks good. Thanks for taking this.
http://hg.mozilla.org/mozilla-central/rev/0a8932a5abdd
http://hg.mozilla.org/mozilla-central/rev/949a0a1ed11a
Status: NEW → RESOLVED
Closed: 11 years ago
Priority: -- → P3
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.2a1
You need to log in before you can comment on or make changes to this bug.