Bug 224610 – HashMapImpl::rehash() should use a version of jsMapHash that cannot throw.

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Mark Lam]

rdar://76698910

[Mark Lam]

Created attachment 426109 [details]
proposed patch.

[Mark Lam]

Created attachment 426133 [details]
proposed patch.

[Yusuke Suzuki]

Comment on attachment 426133 [details]
proposed patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=426133&action=review

> Source/JavaScriptCore/runtime/HashMapImplInlines.h:426
>          EXCEPTION_ASSERT_WITH_MESSAGE(!scope.exception(), "All keys should already be hashed before, so this should not throw because it won't resolve ropes.");

Why don't we just ignore terminated exception in this assertion?
DeferTermination scope inserts two function calls, so this is not a zero-cost. While this function is anyway taking some time, if we replace EXCEPTION_ASSERT with the above idiom, we will see a bad case.
Now, pure EXCEPTION_ASSERT is almost meaningless since TerminatedExcecution exception can be thrown almost at any time.

So, I propose that just ignoring TerminatedExcution in EXCEPTION_ASSERT etc. code, if it is used to ensure that there is not exception, so,

1. Let's replace EXCEPTION_ASSERT with scope.assertNoException if it is used to ensure there is no exceptions.
2. And let's ignore TerminatedExecution exception in assertNoException.

So, we can keep these handling nop in Release build.

[Mark Lam]

(In reply to Yusuke Suzuki from comment #3)
> So, I propose that just ignoring TerminatedExcution in EXCEPTION_ASSERT etc.
> code, if it is used to ensure that there is not exception, so,
> 
> 1. Let's replace EXCEPTION_ASSERT with scope.assertNoException if it is used
> to ensure there is no exceptions.
> 2. And let's ignore TerminatedExecution exception in assertNoException.
> 
> So, we can keep these handling nop in Release build.

For the record, Yusuke and I discussed this offline: the EXCEPTION_ASSERT_WITH_MESSAGE in HashMapImpl::rehash() is not where the TerminationException gets thrown.  Instead, it is thrown from the RETURN_IF_EXCEPTION in jsMapHash().  Yusuke suggested that I add a jsMapHashForAlreadyHashedValue() function that doesn't have this RETURN_IF_EXCEPTION.  I will go with that solution.

[Mark Lam]

Created attachment 426158 [details]
proposed patch.

[Yusuke Suzuki]

Comment on attachment 426158 [details]
proposed patch.

r=me

[Mark Lam]

Thanks for the reviews.  Landed in r276106: <http://trac.webkit.org/r276106>.