Bug 224610 - HashMapImpl::rehash() should use a version of jsMapHash that cannot throw.
Summary: HashMapImpl::rehash() should use a version of jsMapHash that cannot throw.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mark Lam
URL:
Keywords: InRadar
Depends on: 224616 224619
Blocks:
  Show dependency treegraph
 
Reported: 2021-04-15 08:16 PDT by Mark Lam
Modified: 2021-04-15 19:08 PDT (History)
7 users (show)

See Also:


Attachments
proposed patch. (3.30 KB, patch)
2021-04-15 08:38 PDT, Mark Lam
no flags Details | Formatted Diff | Diff
proposed patch. (3.50 KB, patch)
2021-04-15 13:42 PDT, Mark Lam
mark.lam: review-
Details | Formatted Diff | Diff
proposed patch. (5.43 KB, patch)
2021-04-15 17:08 PDT, Mark Lam
ysuzuki: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Lam 2021-04-15 08:16:16 PDT
rdar://76698910
Comment 1 Mark Lam 2021-04-15 08:38:30 PDT
Created attachment 426109 [details]
proposed patch.
Comment 2 Mark Lam 2021-04-15 13:42:02 PDT
Created attachment 426133 [details]
proposed patch.
Comment 3 Yusuke Suzuki 2021-04-15 14:12:22 PDT
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.
Comment 4 Mark Lam 2021-04-15 16:21:59 PDT
(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.
Comment 5 Mark Lam 2021-04-15 17:08:43 PDT
Created attachment 426158 [details]
proposed patch.
Comment 6 Yusuke Suzuki 2021-04-15 17:38:08 PDT
Comment on attachment 426158 [details]
proposed patch.

r=me
Comment 7 Mark Lam 2021-04-15 19:08:35 PDT
Thanks for the reviews.  Landed in r276106: <http://trac.webkit.org/r276106>.