Bug 224412

Summary: WebProcessPool should store Vector<Ref<WebProcessProxy>> instead of Vector<RefPtr>
Product: WebKit Reporter: Alex Christensen <achristensen>
Component: New BugsAssignee: Alex Christensen <achristensen>
Status: RESOLVED FIXED    
Severity: Normal CC: benjamin, cdumez, cmarcelo, darin, ews-watchlist, jer.noble, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Description Alex Christensen 2021-04-10 20:46:29 PDT
WebProcessPool should store Vector<Ref<WebProcessProxy>> instead of Vector<RefPtr>
Comment 1 Alex Christensen 2021-04-10 20:47:25 PDT
Created attachment 425698 [details]
Patch
Comment 2 Chris Dumez 2021-04-10 21:23:07 PDT
Comment on attachment 425698 [details]
Patch

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

R=me with nits

> Source/WebKit/UIProcess/WebProcessPool.cpp:513
> +    Vector<Ref<WebProcessProxy>> processes = m_processes;

Auto ?

> Source/WebKit/UIProcess/WebProcessPool.cpp:904
> +    Vector<Ref<WebProcessProxy>> processes = m_processes;

Auto?

> Source/WebKit/UIProcess/WebProcessPool.cpp:1503
> +    Vector<Ref<WebProcessProxy>> processes = m_processes;

Auto?
Comment 3 Alex Christensen 2021-04-10 23:10:25 PDT
Created attachment 425700 [details]
Patch
Comment 4 Alex Christensen 2021-04-10 23:15:34 PDT
Comment on attachment 425700 [details]
Patch

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

> Source/WebCore/ChangeLog:11
> +        Remove the Ref comparators for Ref<SharedBuffer> which were only used in tests.

This isn't true.
Comment 5 Alex Christensen 2021-04-13 14:54:43 PDT
(In reply to Chris Dumez from comment #2)
> > Source/WebKit/UIProcess/WebProcessPool.cpp:1503
> > +    Vector<Ref<WebProcessProxy>> processes = m_processes;
> 
> Auto?

I feel like it's important to have the type explicit here to indicate that we are copying the Vector<Ref> instead of making a reference or something that wouldn't.
Comment 6 Alex Christensen 2021-04-13 14:54:50 PDT
Created attachment 425918 [details]
Patch
Comment 7 EWS 2021-04-13 15:48:17 PDT
Committed r275916 (236480@main): <https://commits.webkit.org/236480@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 425918 [details].
Comment 8 Radar WebKit Bug Importer 2021-04-13 15:49:31 PDT
<rdar://problem/76614575>
Comment 9 Darin Adler 2021-04-13 16:36:54 PDT
Comment on attachment 425918 [details]
Patch

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

> Source/WTF/wtf/Ref.h:326
> +template<typename T, typename U, typename V, typename W>
> +inline bool operator==(const Ref<T, U>& a, const Ref<V, W>& b)
> +{
> +    return a.ptr() == b.ptr();
> +}
> +
> +template<typename T, typename U, typename V>
> +inline bool operator==(const Ref<T, U>& a, V& b)
> +{
> +    return a.ptr() == &b;
> +}
> +
> +template<typename T, typename U, typename V>
> +inline bool operator==(T& a, const Ref<U, V>& b)
> +{
> +    return &a == b.ptr();
> +}
> +
> +template<typename T, typename U, typename V, typename W>
> +inline bool operator!=(const Ref<T, U>& a, const Ref<V, W>& b)
> +{
> +    return a.ptr() != b.ptr();
> +}
> +
> +template<typename T, typename U, typename V>
> +inline bool operator!=(const Ref<T, U>& a, V& b)
> +{
> +    return a.ptr() != &b;
> +}
> +
> +template<typename T, typename U, typename V>
> +inline bool operator!=(T& a, const Ref<U, V>& b)
> +{
> +    return &a != b.ptr();
> +}

When originally creating Ref<> we intentionally omitted these function templates, because comparing two references with == compares the *values*, not the pointers. So we thought it was ambiguous whether a == b should be a.ptr() == b.ptr() or a.get() == b.get(), which are different.

I know this is handy if you think of Ref as a RefPtr that can never be null. But less logical if you think of it as a kind of reference.
Comment 10 Alex Christensen 2021-04-13 16:39:18 PDT
I could see that confusion when comparing a Ref<T> and a T&.  Do you think it's also confusing when comparing a Ref<T> and another Ref<T>?
Comment 11 Darin Adler 2021-04-13 16:44:22 PDT
(In reply to Alex Christensen from comment #10)
> Do you think
> it's also confusing when comparing a Ref<T> and another Ref<T>?

I did back when we made the original call. Not sure how I feel about it now.
Comment 12 Chris Dumez 2021-04-13 16:46:26 PDT
(In reply to Darin Adler from comment #9)
> Comment on attachment 425918 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=425918&action=review
> 
> > Source/WTF/wtf/Ref.h:326
> > +template<typename T, typename U, typename V, typename W>
> > +inline bool operator==(const Ref<T, U>& a, const Ref<V, W>& b)
> > +{
> > +    return a.ptr() == b.ptr();
> > +}
> > +
> > +template<typename T, typename U, typename V>
> > +inline bool operator==(const Ref<T, U>& a, V& b)
> > +{
> > +    return a.ptr() == &b;
> > +}
> > +
> > +template<typename T, typename U, typename V>
> > +inline bool operator==(T& a, const Ref<U, V>& b)
> > +{
> > +    return &a == b.ptr();
> > +}
> > +
> > +template<typename T, typename U, typename V, typename W>
> > +inline bool operator!=(const Ref<T, U>& a, const Ref<V, W>& b)
> > +{
> > +    return a.ptr() != b.ptr();
> > +}
> > +
> > +template<typename T, typename U, typename V>
> > +inline bool operator!=(const Ref<T, U>& a, V& b)
> > +{
> > +    return a.ptr() != &b;
> > +}
> > +
> > +template<typename T, typename U, typename V>
> > +inline bool operator!=(T& a, const Ref<U, V>& b)
> > +{
> > +    return &a != b.ptr();
> > +}
> 
> When originally creating Ref<> we intentionally omitted these function
> templates, because comparing two references with == compares the *values*,
> not the pointers. So we thought it was ambiguous whether a == b should be
> a.ptr() == b.ptr() or a.get() == b.get(), which are different.
> 
> I know this is handy if you think of Ref as a RefPtr that can never be null.
> But less logical if you think of it as a kind of reference.

I think we went the way of "RefPtr that cannot be null" instead of "kind of a reference" when we made Ref<> copyable too.
Comment 13 Jer Noble 2022-02-25 09:44:41 PST
(In reply to Chris Dumez from comment #12)
> (In reply to Darin Adler from comment #9)
> > Comment on attachment 425918 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=425918&action=review
> > 
> > > Source/WTF/wtf/Ref.h:326
> > > +template<typename T, typename U, typename V, typename W>
> > > +inline bool operator==(const Ref<T, U>& a, const Ref<V, W>& b)
> > > +{
> > > +    return a.ptr() == b.ptr();
> > > +}
> > > +
> > > +template<typename T, typename U, typename V>
> > > +inline bool operator==(const Ref<T, U>& a, V& b)
> > > +{
> > > +    return a.ptr() == &b;
> > > +}
> > > +
> > > +template<typename T, typename U, typename V>
> > > +inline bool operator==(T& a, const Ref<U, V>& b)
> > > +{
> > > +    return &a == b.ptr();
> > > +}
> > > +
> > > +template<typename T, typename U, typename V, typename W>
> > > +inline bool operator!=(const Ref<T, U>& a, const Ref<V, W>& b)
> > > +{
> > > +    return a.ptr() != b.ptr();
> > > +}
> > > +
> > > +template<typename T, typename U, typename V>
> > > +inline bool operator!=(const Ref<T, U>& a, V& b)
> > > +{
> > > +    return a.ptr() != &b;
> > > +}
> > > +
> > > +template<typename T, typename U, typename V>
> > > +inline bool operator!=(T& a, const Ref<U, V>& b)
> > > +{
> > > +    return &a != b.ptr();
> > > +}
> > 
> > When originally creating Ref<> we intentionally omitted these function
> > templates, because comparing two references with == compares the *values*,
> > not the pointers. So we thought it was ambiguous whether a == b should be
> > a.ptr() == b.ptr() or a.get() == b.get(), which are different.
> > 
> > I know this is handy if you think of Ref as a RefPtr that can never be null.
> > But less logical if you think of it as a kind of reference.
> 
> I think we went the way of "RefPtr that cannot be null" instead of "kind of
> a reference" when we made Ref<> copyable too.

This actually caused a real, user facing bug. This changed the behavior of a check inside the EME code to cause a pointer-equality test rather than a content test, and that test now fails, causing key rotations to fail.