Bug 211844 – JSDOMWindowBase m_windowCloseWatchpoints must be Ref<>

• Status: RESOLVED
• Resolution: FIXED
• Component: Bindings

[[e-mail]]

Created attachment 399266 [details]
patch

JSC WatchpointSet is reference counted, however m_windowCloseWatchpoints is just a member variable.

ProxyableAccessCase holds on it, when JSDOMWindowBase is destroyed, m_additionalSet holds a dangling pointer.

By the way, WatchpointSet constructor should be private.

I can not build latest WebKit, please clean up the patch, add other stuffs, then commit it.


Thanks.

[Chris Dumez]

(In reply to [e-mail] from comment #0)
> Created attachment 399266 [details]
> patch
> 
> JSC WatchpointSet is reference counted, however m_windowCloseWatchpoints is
> just a member variable.
> 
> ProxyableAccessCase holds on it, when JSDOMWindowBase is destroyed,
> m_additionalSet holds a dangling pointer.
> 
> By the way, WatchpointSet constructor should be private.
> 
> I can not build latest WebKit, please clean up the patch, add other stuffs,
> then commit it.
> 
> 
> Thanks.

We are unable to approve / commit patches without test cases.

[[e-mail]]

Test case is not practical, it depends on memory access.

First, JSDOMWindowBase must be destroyed before ProxyableAccessCase which holds its m_windowCloseWatchpoints, this step is possible.

Second, since WatchpointSet is reference counted, the memory which has m_windowCloseWatchpoints's reference counter must be 1, otherwise, ProxyableAccessCase does not destroy its m_additionalSet (JSDOMWindowBase's m_windowCloseWatchpoints) when itself is deleted. This step is not practical in test case.

[Yusuke Suzuki]

(In reply to [e-mail] from comment #2)
> Test case is not practical, it depends on memory access.
> 
> First, JSDOMWindowBase must be destroyed before ProxyableAccessCase which
> holds its m_windowCloseWatchpoints, this step is possible.

I think this is possible while I don't come up with the test case.

> 
> Second, since WatchpointSet is reference counted, the memory which has
> m_windowCloseWatchpoints's reference counter must be 1, otherwise,
> ProxyableAccessCase does not destroy its m_additionalSet (JSDOMWindowBase's
> m_windowCloseWatchpoints) when itself is deleted. This step is not practical
> in test case.

The initial m_refCount of RefCounted object is 1. And when wrapping this with Ref<> initially at construction, we do not increment it because of this (this is called "adopt". See adoptRef).
So this does not matter.

[Yusuke Suzuki]

(In reply to Yusuke Suzuki from comment #3)
> (In reply to [e-mail] from comment #2)
> > Test case is not practical, it depends on memory access.
> > 
> > First, JSDOMWindowBase must be destroyed before ProxyableAccessCase which
> > holds its m_windowCloseWatchpoints, this step is possible.
> 
> I think this is possible while I don't come up with the test case.

After looking into this deeply, I think this does not matter. ProxyableAccessCase has Structure, and this Structure will point to JSDOMWindow. So, as long as ProxyableAccessCase is alive, JSDOMWindow is also alive.

But anyway, using RefCounted object without RefCount-creation is not good. I'll fix it.

[Yusuke Suzuki]

Created attachment 399319 [details]
Patch

[Mark Lam]

Comment on attachment 399319 [details]
Patch

r=me

[EWS]

Committed r261668: <https://trac.webkit.org/changeset/261668>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 399319 [details].

[Radar WebKit Bug Importer]

<rdar://problem/63210610>