Bug 188491 - Break reference cycle in ErrorEvent by using JSValueInWrappedObject
Summary: Break reference cycle in ErrorEvent by using JSValueInWrappedObject
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yusuke Suzuki
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-08-12 13:54 PDT by Yusuke Suzuki
Modified: 2018-08-13 09:47 PDT (History)
10 users (show)

See Also:


Attachments
Patch (12.07 KB, patch)
2018-08-12 13:57 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (14.26 KB, patch)
2018-08-12 14:25 PDT, Yusuke Suzuki
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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 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.