Bug 188491

Summary:

Break reference cycle in ErrorEvent by using JSValueInWrappedObject

Product:

WebKit

Reporter:

Yusuke Suzuki <ysuzuki>

Component:

New Bugs

Assignee:

Yusuke Suzuki <ysuzuki>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

bfulgham, cdumez, darin, dbates, esprehn+autocc, ews-watchlist, kangil.han, kondapallykalyan, tsavell, webkit-bug-importer

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
Patch	none
Patch	darin: review+

Description Yusuke Suzuki 2018-08-12 13:54:16 PDT

ErrorEvent should not hold error strongly

Comment 1 Yusuke Suzuki 2018-08-12 13:57:35 PDT

Created attachment 346988 [details]
Patch

Comment 2 Yusuke Suzuki 2018-08-12 14:25:36 PDT

Created attachment 346989 [details]
Patch

Comment 3 Darin Adler 2018-08-12 14:42:05 PDT

Comment on attachment 346989 [details]
Patch

I don’t fully understand the implications of aerialiing an object that could be changed after its serialized. Or if we are doing the right thing when there are multiple worlds involved. But that’s not specific to this patch, which is applying a pattern we already follow elsewhere.

Comment 4 Darin Adler 2018-08-12 14:43:58 PDT

Title of bug is not exactly right since it does need to hold the error “strongly” in the sense that its a reference that keeps the error object alive, just needs to do it in a way that is garbage collection compatible and doesn’t lead to reference cycles and storage leaks!

Comment 5 Yusuke Suzuki 2018-08-12 17:02:25 PDT

Committed r234789: <https://trac.webkit.org/changeset/234789>

Comment 6 Radar WebKit Bug Importer 2018-08-12 17:03:26 PDT

<rdar://problem/43210312>

Comment 7 Truitt Savell 2018-08-13 08:47:11 PDT

Looks like this revision <https://trac.webkit.org/changeset/234789> 

has caused build failure on Windows release https://build.webkit.org/builders/Apple%20Win%20Release%20%28Build%29/builds/11156

error stdio:
https://build.webkit.org/builders/Apple%20Win%20Release%20%28Build%29/builds/11156/steps/compile-webkit/logs/stdio

Comment 8 Darin Adler 2018-08-13 09:47:39 PDT

Seems highly unlikely this change directly caused that build failure; maybe due to adding a new unified source file? The failure is a link error about export of a symbol affecting units tests. We should get someone to dive deeper into it. A rollout would be OK but this seems really peculiar.