To diagnose a hang inside the m_callbacks in WebPageProxy, assert that callbackID is not 0 or -1 in IPC message encode/decode code
Created attachment 313771 [details] Adds assertion
Created attachment 313773 [details] Fixed iOS build
Created attachment 313774 [details] WPE build fix attempt
Created attachment 313775 [details] Another WPE build fix attempt
Created attachment 313776 [details] Yet another WPE build fix attempt
Created attachment 313777 [details] More GTK+ and WPE build fixes
Created attachment 313779 [details] Another GTK+ and WPE build fixes
Created attachment 313780 [details] Yet another GTK+ and WPE build fixes
Created attachment 313781 [details] Even more GTK+ and WPE build fixes
Created attachment 313782 [details] Another GTK+ build fix
<rdar://problem/32964323>
Comment on attachment 313782 [details] Another GTK+ build fix View in context: https://bugs.webkit.org/attachment.cgi?id=313782&action=review > Source/WebKit2/ChangeLog:47 > + (WebKit::SpecificCallbackMap): Added. Used in WebCookieManagerProxy. In earlier work, we replaced specific maps with generic ones. Why keep the ones in WebCookieManagerProxy separate? > Source/WebKit2/ChangeLog:646 > +>>>>>>> .r218782 Stray conflict marker.
(In reply to mitz from comment #12) > Comment on attachment 313782 [details] > Another GTK+ build fix > > View in context: > https://bugs.webkit.org/attachment.cgi?id=313782&action=review > > > Source/WebKit2/ChangeLog:47 > > + (WebKit::SpecificCallbackMap): Added. Used in WebCookieManagerProxy. > > In earlier work, we replaced specific maps with generic ones. Why keep the > ones in WebCookieManagerProxy separate? Okay, I wasn't aware of that refactoring effort. Fixed. > > Source/WebKit2/ChangeLog:646 > > +>>>>>>> .r218782 > > Stray conflict marker. Removed.
Created attachment 313820 [details] Updated per Dan's comments
We don’t really need this anymore, do we? I think adding MESSAGE_CHECKs on UI process side for hardening would likely suffice to protect against this in the future.
(In reply to Chris Dumez from comment #15) > We don’t really need this anymore, do we? I think adding MESSAGE_CHECKs on > UI process side for hardening would likely suffice to protect against this > in the future. I think it's still useful to wrap CallbackID in a class so that we can easily add diagnostics in the future. For example, we should really be making the decode() function fail when a callbackID is not optional and it's 0 or -1.
Comment on attachment 313820 [details] Updated per Dan's comments View in context: https://bugs.webkit.org/attachment.cgi?id=313820&action=review > Source/WebKit2/Shared/OptionalCallbackID.h:62 > + static ALWAYS_INLINE bool isValidCallbackID(uint64_t rawId) > + { > + return !HashTraits<uint64_t>::isDeletedValue(rawId); > + } Why do we consider emptyValue() to be valid? Isn't that the bug we're trying to catch?
(In reply to Geoffrey Garen from comment #17) > Comment on attachment 313820 [details] > Updated per Dan's comments > > View in context: > https://bugs.webkit.org/attachment.cgi?id=313820&action=review > > > Source/WebKit2/Shared/OptionalCallbackID.h:62 > > + static ALWAYS_INLINE bool isValidCallbackID(uint64_t rawId) > > + { > > + return !HashTraits<uint64_t>::isDeletedValue(rawId); > > + } > > Why do we consider emptyValue() to be valid? Isn't that the bug we're trying > to catch? For OptionalCallbackID, we use 0 to mean it's not set. I have a separate release assertion when the value is used like WTF::Optional.
Comment on attachment 313820 [details] Updated per Dan's comments View in context: https://bugs.webkit.org/attachment.cgi?id=313820&action=review > Source/WebKit2/Shared/CallbackID.h:35 > +namespace WTF { > + > +struct CallbackIDHash; > + > +} I don't usually see empty lines between namespace and forward decls like this. > Source/WebKit2/UIProcess/GenericCallback.h:209 > + // FIXME: Every caller should pass in activityToken > + template<typename... T> > + CallbackID put(Function<void(T...)>&& function) > + { > + auto callback = GenericCallbackType<sizeof...(T), T...>::type::create(WTFMove(function)); > + return put(WTFMove(callback)); > + } Is it okay to punt on this FIXME right now?
(In reply to Brady Eidson from comment #19) > Comment on attachment 313820 [details] > Updated per Dan's comments > > View in context: > https://bugs.webkit.org/attachment.cgi?id=313820&action=review > > > Source/WebKit2/Shared/CallbackID.h:35 > > +namespace WTF { > > + > > +struct CallbackIDHash; > > + > > +} > > I don't usually see empty lines between namespace and forward decls like > this. > > > Source/WebKit2/UIProcess/GenericCallback.h:209 > > + // FIXME: Every caller should pass in activityToken > > + template<typename... T> > > + CallbackID put(Function<void(T...)>&& function) > > + { > > + auto callback = GenericCallbackType<sizeof...(T), T...>::type::create(WTFMove(function)); > > + return put(WTFMove(callback)); > > + } > > Is it okay to punt on this FIXME right now? I wanted to fix it in a separate patch at least to reduce the chance of this patch introducing a new regression.
(In reply to Ryosuke Niwa from comment #20) > (In reply to Brady Eidson from comment #19) > > Comment on attachment 313820 [details] > > Updated per Dan's comments > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=313820&action=review > > > > > Source/WebKit2/Shared/CallbackID.h:35 > > > +namespace WTF { > > > + > > > +struct CallbackIDHash; > > > + > > > +} > > > > I don't usually see empty lines between namespace and forward decls like > > this. > > > > > Source/WebKit2/UIProcess/GenericCallback.h:209 > > > + // FIXME: Every caller should pass in activityToken > > > + template<typename... T> > > > + CallbackID put(Function<void(T...)>&& function) > > > + { > > > + auto callback = GenericCallbackType<sizeof...(T), T...>::type::create(WTFMove(function)); > > > + return put(WTFMove(callback)); > > > + } > > > > Is it okay to punt on this FIXME right now? > > I wanted to fix it in a separate patch at least to reduce the chance of this > patch introducing a new regression. Could we file a bug and include it in the comment?
(In reply to Brady Eidson from comment #21) > (In reply to Ryosuke Niwa from comment #20) > > (In reply to Brady Eidson from comment #19) > > > Comment on attachment 313820 [details] > > > Updated per Dan's comments > > > > > > View in context: > > > https://bugs.webkit.org/attachment.cgi?id=313820&action=review > > > > > > > Source/WebKit2/Shared/CallbackID.h:35 > > > > +namespace WTF { > > > > + > > > > +struct CallbackIDHash; > > > > + > > > > +} > > > > > > I don't usually see empty lines between namespace and forward decls like > > > this. > > > > > > > Source/WebKit2/UIProcess/GenericCallback.h:209 > > > > + // FIXME: Every caller should pass in activityToken > > > > + template<typename... T> > > > > + CallbackID put(Function<void(T...)>&& function) > > > > + { > > > > + auto callback = GenericCallbackType<sizeof...(T), T...>::type::create(WTFMove(function)); > > > > + return put(WTFMove(callback)); > > > > + } > > > > > > Is it okay to punt on this FIXME right now? > > > > I wanted to fix it in a separate patch at least to reduce the chance of this > > patch introducing a new regression. > > Could we file a bug and include it in the comment? Filed https://bugs.webkit.org/show_bug.cgi?id=174007 to track this. Thanks for the review.
Committed r218985: <http://trac.webkit.org/changeset/218985>