[webkit-reviews] review denied: [Bug 208642] REGRESSION: (r257905) [ Mac wk2 Debug ] ASSERTION FAILED: !m_isolatedWorld->isNormal() || m_wrapper || !m_jsFunction : [Attachment 392702] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Fri Mar 6 08:57:52 PST 2020


Alexey Proskuryakov <ap at webkit.org> has denied Yusuke Suzuki
<ysuzuki at apple.com>'s request for review:
Bug 208642: REGRESSION: (r257905) [ Mac wk2 Debug ] ASSERTION FAILED:
!m_isolatedWorld->isNormal() || m_wrapper || !m_jsFunction
https://bugs.webkit.org/show_bug.cgi?id=208642

Attachment 392702: Patch

https://bugs.webkit.org/attachment.cgi?id=392702&action=review




--- Comment #35 from Alexey Proskuryakov <ap at webkit.org> ---
Comment on attachment 392702
  --> https://bugs.webkit.org/attachment.cgi?id=392702
Patch

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

Since the change only makes the assertion stronger, it is good as far as I'm
concerned. But tests are now asserting, so mechanically r-.

> Source/WebCore/ChangeLog:12
> +	      GC, we cannot say such an condition. Even if m_wrapper is dead,
m_jsFunction can be alive by various reasons: conservative
> +	      GC finds it, user code stores this function somewhere reachable
from the root, etc. The correct thing is `m_wrapper` must

There are a couple things here that make me a little skeptical, from a position
of someone who only touched this a long time ago and is absolutely not an
expert.

1. If this assertion can misfire due to conservative GC finding it, why haven't
we ever seen it fire without a real bug?

2. The assertion would fire if user code stored the function somewhere, so it's
essentially saying that user code must not do that. That isn't a bug in the
assertion necessarily. Is there even a reason for user code to do this?

3. Checking m_jsFunction for being null is not the same as checking whether the
function is alive. Once can just assign null to the member, and arguably should
do this without waiting for GC. Is it even right that m_jsFunction is a weak
pointer?

> Source/WebCore/ChangeLog:15
> +	   2. The comments about "zombie m_jsFunctions" is wrong. We are using
JSC::Weak<>. So if the object gets collected, it returns
> +	      nullptr, not getting a zombie pointer.

Yes, must have been written before these were weak pointers.

> Source/WebCore/ChangeLog:17
> +	   3. We are emitting write-barrier in a wrong order. In the heavily
stressed scenario, it is possible that concurrent marking
> +	      scans JSEventListener just after we emit the write-barrier, and
this marking misses the assigned value. We must emit

Great find! Is there any test that hits this, at least in StressGC
configuration?

> Source/WebCore/ChangeLog:21
> +	   Still keeping normal world check. But this looks also wrong. The
assertion is allowing non-normal world to get dead m_wrapper.

Yes, pretty sure it's been understood to be a dirty hack when added.
Interaction between worlds and event listeners was bogus back then, and I don't
know if it has been improved since.


More information about the webkit-reviews mailing list