WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
51508
WebProcess should be terminated if invalid frameIDs are passed to the UIProcess
https://bugs.webkit.org/show_bug.cgi?id=51508
Summary
WebProcess should be terminated if invalid frameIDs are passed to the UIProcess
Sam Weinig
Reported
2010-12-22 16:57:53 PST
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.
Attachments
Patch
(23.09 KB, patch)
2010-12-22 16:59 PST
,
Sam Weinig
darin
: review+
Details
Formatted Diff
Diff
View All
Add attachment
proposed patch, testcase, etc.
Sam Weinig
Comment 1
2010-12-22 16:59:40 PST
Created
attachment 77280
[details]
Patch
Darin Adler
Comment 2
2010-12-22 17:43:25 PST
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.
Sam Weinig
Comment 3
2010-12-22 17:58:00 PST
Landed in
r74528
.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug