The webProcess should be terminated if invalid frameIDs are passed to the UIProcess. We should fix this and introduce a mechanism to do this in general.
Created attachment 77280 [details] Patch
Comment on attachment 77280 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=77280&action=review > WebKit2/Platform/CoreIPC/Connection.cpp:54 > + , m_inDispatchMessageCount(0) I’m not sure this count is helpful. It’s just for the assertion? > WebKit2/Platform/CoreIPC/Connection.h:65 > + if (!(assertion)) { \ > + ASSERT(assertion); \ This evaluates the assertion twice in debug builds. It’s not obvious how to avoid that. We can probably live with this for a while, since it affects the macro not all the macro call sites. > WebKit2/UIProcess/WebPageProxy.cpp:76 > +// FIXME: This should be generalized and moved to a different place. Please do remove that FIXME. > WebKit2/UIProcess/WebPageProxy.cpp:891 > process()->frameCreated(frameID, m_mainFrame.get()); There is no need for the get() here. You can pass a RefPtr to a PassRefPtr without it. Unless frameCreated takes a raw pointer. In which case I wonder why owns the subframe proxy below. > WebKit2/UIProcess/WebPageProxy.cpp:902 > + RefPtr<WebFrameProxy> subFrame = WebFrameProxy::create(this, frameID); > + > + // Add the frame to the process wide map. > + process()->frameCreated(frameID, subFrame.get()); I probably would have done this in one line without the local variable. Also, if frameCreated takes ownership, this should be release() rather than get(). Since subframe is a word, I would not capitalize the “f” in this local variable or in function names. > WebKit2/UIProcess/WebProcessProxy.cpp:47 > +template<typename HashMap> > +static inline bool isGoodMapKey(const typename HashMap::KeyType& key) Could just name this isGoodKey since the map will be right there was the template argument. As we discussed, this would be great as a public HashMap static member function. > WebKit2/UIProcess/WebProcessProxy.cpp:49 > + return key != HashTraits<typename HashMap::KeyType>::emptyValue() && !HashTraits<typename HashMap::KeyType>::isDeletedValue(key); This implementation may be as good as we can achieve, but it gives the wrong answer for maps using non-default traits. We should consider making the trait typedefs public so they can be used for things like this.
Landed in r74528.