Bug 224412 – WebProcessPool should store Vector<Ref<WebProcessProxy>> instead of Vector<RefPtr>

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Alex Christensen]

WebProcessPool should store Vector<Ref<WebProcessProxy>> instead of Vector<RefPtr>

[Alex Christensen]

Created attachment 425698 [details]
Patch

[Chris Dumez]

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?

[Alex Christensen]

Created attachment 425700 [details]
Patch

[Alex Christensen]

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.

[Alex Christensen]

(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.

[Alex Christensen]

Created attachment 425918 [details]
Patch

[EWS]

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].

[Radar WebKit Bug Importer]

<rdar://problem/76614575>

[Darin Adler]

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.

[Alex Christensen]

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>?

[Darin Adler]

(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.

[Chris Dumez]

(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.

[Jer Noble]

(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.