Closed Bug 334180 Opened 15 years ago Closed 15 years ago

pldhash/jsdhash should assert that ops don't mutate the table

Categories

(Core :: XPCOM, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla1.8.1alpha1

People

(Reporter: dbaron, Assigned: dbaron)

References

Details

(Keywords: fixed1.8.1, Whiteboard: [patch])

Attachments

(4 files, 2 obsolete files)

pldhash should assert that its ops don't recursively mutate the table by checking that table->entryCount, table->removedCount, and (maybe) table->generation do not change across calls to ops.  See bug 334177 for a crash that this might have caught sooner.

I still haven't thought of a good way to do this without making the code messy, but I'll try to think about it.  I expect I'll introduce a macro for calling ops, though.
Then again, much more useful to assert on the inner mutation by adding a DEBUG-only boolean member to PLDHashTable.
Does the JSDHashTable struct attempt to preserve binary compatibility in a way that would prevent adding a DEBUG_only member?

(Also, bug 313309 comment 20 is a pain.)
Can someone please resync js and pl here, by hacking on js/src/plify_jsdhash.sed?  That is the rule, please follow it per the generated comment in pldhash.*.

DEBUG-only binary incompatibility is ok by me.

/be
Attached patch patch (obsolete) — Splinter Review
I'll manually re-merge bsmedberg's changes when landing, if that's still needed.
Attachment #218710 - Flags: review?(brendan)
Attachment #218710 - Flags: approval-branch-1.8.1?(brendan)
Status: NEW → ASSIGNED
Priority: -- → P1
Whiteboard: [patch]
Target Milestone: --- → mozilla1.8.1alpha1
I'm not so happy with DEBUG-only binary incompatibility: we include pldhash in the XPCOM glue static lib, and it's perfectly reasonable for DEBUG client code to link against non-DEBUG xpcomglue_s
(In reply to comment #6)
> I'm not so happy with DEBUG-only binary incompatibility: we include pldhash in
> the XPCOM glue static lib, and it's perfectly reasonable for DEBUG client code
> to link against non-DEBUG xpcomglue_s

That's actually not a problem in this case since it's at the end of the struct; the only problems would be if:
 (1) client code tried to poke at the member variable explicitly, #ifdef DEBUG, along with a non-debug libxpcom.so
 (2) non-DEBUG component using the struct rather than PL_NewDHashTable along with a DEBUG libxpcom.so

I think (2) is actually a real problem, though.
Comment on attachment 218710 [details] [diff] [review]
patch

I'll do the less-useful assertions that give the assertion on the way out.
Attachment #218710 - Flags: review?(brendan)
Attachment #218710 - Flags: review-
Attachment #218710 - Flags: approval-branch-1.8.1?(brendan)
(In reply to comment #6)
> I'm not so happy with DEBUG-only binary incompatibility: we include pldhash in
> the XPCOM glue static lib, and it's perfectly reasonable for DEBUG client code
> to link against non-DEBUG xpcomglue_s

FWIW, I've been bitten by mixing DEBUG and non-DEBUG libs and it's a real pain to figure out what went wrong. For me, at least, crashes began happening because of stack corruption and it was difficult to find exactly what was causing it.

I realize that's not exactly a strong argument against the DEBUG-only variable, but if it happened to me then it will probably happen to others, especially once people begin using the gecko SDK more frequently.
I'm personally not worried about 2), since we're talking about static linkage: it doesn't affect the SDK at all, since that would not be compiled with DEBUG in any released configuration.
Attachment #218712 - Flags: review?(brendan) → review+
I can just allocate an extra word in the entry store and cover it up with macros.
Attached patch patchSplinter Review
Attachment #218710 - Attachment is obsolete: true
Attachment #218790 - Flags: review?(brendan)
Comment on attachment 218790 [details] [diff] [review]
patch

>+/*
>+ * The following DEBUG-only code is used to assert that calls to one of
>+ * table->ops or to an enumerator do not cause re-entry into a call that
>+ * can mutate the table.  The recursion level is stored in additional
>+ * space allocated at the end of the entry store to avoid changing
>+ * issues when mixing DEBUG and non-DEBUG components.
>+ */
>+#ifdef DEBUG
>+
>+#define RECURSION_LEVEL(table_) (*(uint32*)(table_->entryStore + \
>+  JS_DHASH_TABLE_SIZE(table_) * table_->entrySize))
>+
>+#define ENTRY_STORE_EXTRA sizeof(uint32)
>+#define INCREMENT_RECURSION_LEVEL(table_) (++RECURSION_LEVEL(table))
>+#define DECREMENT_RECURSION_LEVEL(table_) (--RECURSION_LEVEL(table))

Thanks, only nit is to format the above (and the #else counterparts) like so:

#define RECURSION_LEVEL(table_) (*(uint32*)(table_->entryStore +              \
                                            JS_DHASH_TABLE_SIZE(table_) *     \
                                            table_->entrySize))

#define ENTRY_STORE_EXTRA                   sizeof(uint32)
#define INCREMENT_RECURSION_LEVEL(table_)   (++RECURSION_LEVEL(table))
#define DECREMENT_RECURSION_LEVEL(table_)   (--RECURSION_LEVEL(table))

/be
Attachment #218790 - Flags: review?(brendan) → review+
And fix the s/table/table_/ which I notice is needed :-)
Attachment #218790 - Flags: approval-branch-1.8.1?(brendan)
Checked in to trunk.
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Comment on attachment 218790 [details] [diff] [review]
patch

a=me for 1.8 branch, with the nits and macro formal name fixed.

/be
Attachment #218790 - Flags: approval-branch-1.8.1?(brendan) → approval-branch-1.8.1+
This is what I landed on the trunk.
Checked in to MOZILLA_1_8_BRANCH.
Keywords: fixed1.8.1
This causes new warnings in optimized builds:

gcc -o Linux_All_OPT.OBJ/jsdhash.o -c -Wall -Wno-format -O -DXP_UNIX -DSVR4 -DSYSV -D_BSD_SOURCE -DPOSIX_SOURCE -DHAVE_LOCALTIME_R -DX86_LINUX  -UDEBUG -DNDEBUG -UDEBUG_ -DEDITLINE -ILinux_All_OPT.OBJ  jsdhash.c
jsdhash.c: In function `JS_DHashTableFinish':
jsdhash.c:364: warning: statement with no effect
jsdhash.c:382: warning: statement with no effect
jsdhash.c: In function `JS_DHashTableOperate':
jsdhash.c:537: warning: statement with no effect
jsdhash.c:631: warning: statement with no effect
jsdhash.c: In function `JS_DHashTableEnumerate':
jsdhash.c:663: warning: statement with no effect
jsdhash.c:711: warning: statement with no effect
Yeah, I meant to switch back to this; I was using them as expressions at one point.
Attachment #219033 - Flags: review?(brendan)
Attachment #219033 - Flags: approval-branch-1.8.1?(brendan)
Attachment #219033 - Attachment is obsolete: true
Attachment #219034 - Flags: review?(brendan)
Attachment #219034 - Flags: approval-branch-1.8.1?(brendan)
Attachment #219033 - Flags: review?(brendan)
Attachment #219033 - Flags: approval-branch-1.8.1?(brendan)
Comment on attachment 219034 [details] [diff] [review]
make increment and decrement be statements rather than expressions

It's simpler just to (void) cast the real macro defs, and make the stub-macros expand to /* nothing */ rather than (void)0.

/be
Comment on attachment 219034 [details] [diff] [review]
make increment and decrement be statements rather than expressions

Sorry I didn't mention this, I always (void) cast if possible to make expression statements not invite warnings, else use JS_BEGIN_MACRO/JS_END_MACRO.  See jslock.h for an example.

/be
Comment on attachment 218712 [details] [diff] [review]
backport pldhash -> jsdhash of win32 fastcall support, rev. 1 (checked in)

mozilla/js/src/jsdhash.h 	1.33
Attachment #218712 - Attachment description: backport pldhash -> jsdhash of win32 fastcall support, rev. 1 → backport pldhash -> jsdhash of win32 fastcall support, rev. 1 (checked in)
Comment on attachment 218863 [details] [diff] [review]
final trunk patch

> PLDHashEntryHdr * PL_DHASH_FASTCALL
> PL_DHashTableOperate(PLDHashTable *table, const void *key, PLDHashOperator op)
> {
 ...
>+    NS_ASSERTION(op == PL_DHASH_LOOKUP || RECURSION_LEVEL(table) == 0,
>+                 "op == PL_DHASH_LOOKUP || RECURSION_LEVEL(table) == 0");
>+    INCREMENT_RECURSION_LEVEL(table);
>+

I can trigger this assertion consistently in a fresh 1.8 branch firefox build during XPCOM shutdown after selecting a new profile from the profile manager. Should this be a new bug?
(In reply to comment #25)
> I can trigger this assertion consistently in a fresh 1.8 branch firefox build
> during XPCOM shutdown after selecting a new profile from the profile manager.
> Should this be a new bug?

If it's not the same as bug 334605, yes.
(In reply to comment #26)
> If it's not the same as bug 334605, yes.

It was :)
Attachment #219034 - Flags: approval-branch-1.8.1?(brendan)
Comment on attachment 219034 [details] [diff] [review]
make increment and decrement be statements rather than expressions

Fixed otherwise ((void)[01] expressions).

/be
Attachment #219034 - Flags: review?(brendan)
You need to log in before you can comment on or make changes to this bug.