Summary: | WebProcessPool should store Vector<Ref<WebProcessProxy>> instead of Vector<RefPtr> | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Alex Christensen <achristensen> | ||||||||
Component: | New Bugs | Assignee: | 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
Alex Christensen
2021-04-10 20:46:29 PDT
Created attachment 425698 [details]
Patch
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? Created attachment 425700 [details]
Patch
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. (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. Created attachment 425918 [details]
Patch
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 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 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>? (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. (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. (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. |