74883 - JSDHashTableOps.initEntry patch now crashes @ chrome reg.

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

I've got a patch for people to peek at while I build tests.

Comment 1

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Attachment: add JS_DHASH_RAWADD and documentation, and fix missed jsdhash->pldhash in plify_jsdhash.sed

Comment 2

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Brendan says he doesn't like this approach, and also says that he has a
counter-proposal.

Comment 3

[Brendan Eich [:brendan]]

That patch can't work: JS_DHASH_LOOKUP doesn't set entry->keyHash, so the random
logic based on op that loads from ((JSDHashEntry *)key)->keyHash will load 0.

I don't see a need for raw-add, unless there is a raw-lookup that returns the
keyHash it computes.  But even then, I think code that wants to look, but not
touch, and then add only if the lookup returned a free entry, should instead
just add.

Given the all-zeroes state of entry storage, after adding a brand new entry,
only keyHash will be non-zero, and the caller of JS_DHashTableOperate can
inspect other members of its JSDHashEntryHdr extension struct looking for a zero
where no initialized entry would have a zero.  If zero is ambiguous, there is no
issue.  If you need to construct (C++) or otherwise initialize an entry before
keyHash is set, the initEntry hook in the next patch can be used.

Shaver, you buy this?

/be

Comment 4

[Brendan Eich [:brendan]]

Attachment: my proposed fix (some whitespace and similar nit-picking in addition to initEntry and ChangeTable diffs)

Comment 5

[Brendan Eich [:brendan]]

Forgot to say that adding an entry for key, where key already has an entry, does
no harm.  That finishes the case analysis: cases that want to lookup and add iff
not-found should just add.

To ensure that initEntry is not called more than once per add (in the case where
the table needs to grow), ChangeTable now takes a JSDHashEntryHdr **findEntry
in/out param, which may be null if no entry needs to be tracked across the copy
to the grown table.

/be

Comment 6

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Looks pretty good.  Can I convince you to document that use case in jsdhash.h's
JSDHashTableOperate comment?

sr=shaver.

Comment 7

[Chris Waterson]

So the initEntry stuff is added to support placement new, right? What is the
sequence of events that occurs when you rehash the table? I'm just trying to
think how a C++ object derived from PLDHashEntryHdr would be implemented.

  allocTable    allocate raw bytes with malloc, no ctors run.
  freeTable     free raw bytes with free, no dtors run.
  initEntry     call placement new using default ctor
  moveEntry     call placement new using copy ctor, run dtor
                on old entry? (or will that be clearEntry'd?)
  clearEntry    run dtor which should call PLDHashClearEntryStub()
  finalize      no-op

Does that seem correct? If so, I understand what's going on, and r=waterson.

Comment 8

[Brendan Eich [:brendan]]

  allocTable    allocate raw bytes with malloc, no ctors run.
  freeTable     free raw bytes with free, no dtors run.

Right and right.

  initEntry     call placement new using default ctor

Check-a-roonie.

  moveEntry     call placement new using copy ctor, run dtor
                on old entry? (or will that be clearEntry'd?)

Right the first time -- moveEntry "moves", it does not "copy".  Running just the
copy ctor but not the dtor would be a cp, and need a clearEntry "rm" to ape mv,
but the pldhash code does not call clearEntry on the old table entry.  Move it!

  clearEntry    run dtor which should call PLDHashClearEntryStub()

Almost -- you can use PL_DHashClearEntryStub if you've nothing better to do, but
if you specialize your own clearEntry, no need to call the stub.  Stub here does
not mean "base class impl, always delegate to me last".

  finalize      no-op

Right.

I'll comment all this better, stick in a final patch, and go when it's ready.

/be

Comment 9

[Brendan Eich [:brendan]]

Attachment: final patch, per review comments

Comment 10

[Brendan Eich [:brendan]]

Fix checked in.

/be

Comment 11

[Phil Schwartau]

Marking Verified - 

Comment 12

[R.K.Aa.]

linux CVS is crashing all over the place for me now:
nsCacheEntryHashTable::GetKey () from dist/bin/components/libnkcache.so
Will clobber.

Comment 13

[Brendan Eich [:brendan]]

I am a bucket-head.  Clients who use initEntry work fine when adding and the
table grows, but old-style clients die.  Better patch coming up.

/be

Comment 14

[Brendan Eich [:brendan]]

Attachment: much simpler fix, I am filled with shame for not writing this up front!

Comment 15

[Brendan Eich [:brendan]]

My stepwise refinement of JS_DHashTableOperate went awry.  Once I had factored
ChangeTable out of inline code in JS_DHashTableOperate, I should have simplified
the JS_DHASH_ADD and _REMOVE cases to call SearchTable and ChangeTable in the
right order for each case.

gordon, waterson, gimme r= and sr= love.

/be

Comment 16

[Brendan Eich [:brendan]]

Attachment: let's try that again (ignore last patch please)

Comment 17

[Chris Waterson]

I'm crashing here at startup during chrome registry; your patch fixes that. 
I'm almost sure this will be a smoketest blocker. [s]r=waterson

Comment 18

[Brendan Eich [:brendan]]

waterson: were you up to date in xpcom/ds before trying the latest patch?  If
not, that might account for the crash during chrome registry.  What was the
stack backtrace?

/be

Comment 19

[gordon]

Brendan, I like it; less "clever" and easier to follow (I always paused a moment 
to remember what biasedDeltaLog2 meant).  If you've run this with the cache, then 
r=gordon.

Comment 20

[Chris McAfee]

note to smoketest watchers, we will open the tree for this, but keep smoketest
status
and will hold the tree tomorrow for this if needed.  -mcafee & jj

Comment 21

[Brendan Eich [:brendan]]

My patch works for me, I breakpoint and run mozilla and see lots of people using
pldhash these days!

/be

Comment 22

[Brendan Eich [:brendan]]

Waterson got a libpr0n bug that blocked the tree opening, diagnosed enough to
bounce to pavlov, which gave me an opening to check in the fix.  /me slinks away
now with his head hung low

/be

Comment 23

[Brendan Eich [:brendan]]

*** Bug 81283 has been marked as a duplicate of this bug. ***

Comment 24

[Phil Schwartau]

Marking Verified -