Move security logic out of the JavaScript binding for location into the DOM class
Created attachment 77630 [details] Patch
Attachment 77630 [details] did not build on chromium: Build output: http://queues.webkit.org/results/7255240
Comment on attachment 77630 [details] Patch I'm taking down the patch so I can add the include and so fix the Chromium build (at least) and also investigate some regression test failures that indicate I made a small mistake somewhere.
Adam, this is the work you recently suggested.
Attachment 77630 [details] did not build on chromium: Build output: http://queues.webkit.org/results/7270232
Created attachment 77651 [details] Patch
Comment on attachment 77651 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=77651&action=review Very nice. I've suggested some FIXME comments below. It's amazing to me how much of this machinery was redundant. Thanks for working on this code. I'll follow up with the V8 change once this patch lands. > WebCore/bindings/js/JSDOMBinding.cpp:701 > Frame* toDynamicFrame(ExecState* exec) > { > - return JSBindingState(exec).firstFrame(); > + return firstDOMWindow(exec)->frame(); > } We should probably rename this function to something involving "first" to be consistent with our new names. > WebCore/bindings/js/JSDOMBinding.cpp:706 > bool processingUserGesture() > { > - return JSBindingState(JSMainThreadExecState::currentState()).processingUserGesture(); > -} > - > -KURL completeURL(ExecState* exec, const String& relativeURL) > -{ > - JSBindingState state(exec); > - return completeURL(&state, relativeURL); > + return ScriptController::processingUserGesture(); > } We should probably inline this function into its call sites. > WebCore/page/DOMWindow.cpp:1612 > -void DOMWindow::setLocation(const String& urlString, DOMWindow* activeWindow, DOMWindow* firstWindow) > +void DOMWindow::setLocation(const String& urlString, DOMWindow* activeWindow, DOMWindow* firstWindow, > + SetLocationLocking locking) A line break! In WebKit! :) > WebCore/page/Location.cpp:262 > + // FIXME: It's not clear this cross-origin security check is valuable. > + // We allow one page to change the location of another. Why block attempts to reload? > + // Other location operations simply block use of JavaScript URLs cross origin. Maybe it has to do with re-POSTing form data? The other location operations can only generate GET requests.
(In reply to comment #7) > > WebCore/bindings/js/JSDOMBinding.cpp:701 > > Frame* toDynamicFrame(ExecState* exec) > > { > > - return JSBindingState(exec).firstFrame(); > > + return firstDOMWindow(exec)->frame(); > > } > > We should probably rename this function to something involving "first" to be consistent with our new names. My intention is to delete this entirely ASAP. > > WebCore/bindings/js/JSDOMBinding.cpp:706 > > bool processingUserGesture() > > { > > - return JSBindingState(JSMainThreadExecState::currentState()).processingUserGesture(); > > -} > > - > > -KURL completeURL(ExecState* exec, const String& relativeURL) > > -{ > > - JSBindingState state(exec); > > - return completeURL(&state, relativeURL); > > + return ScriptController::processingUserGesture(); > > } > > We should probably inline this function into its call sites. The reason I didn't in this patch was concern over the header dependency on ScriptController. > > WebCore/page/DOMWindow.cpp:1612 > > -void DOMWindow::setLocation(const String& urlString, DOMWindow* activeWindow, DOMWindow* firstWindow) > > +void DOMWindow::setLocation(const String& urlString, DOMWindow* activeWindow, DOMWindow* firstWindow, > > + SetLocationLocking locking) > > A line break! In WebKit! :) LOL. > > WebCore/page/Location.cpp:262 > > + // FIXME: It's not clear this cross-origin security check is valuable. > > + // We allow one page to change the location of another. Why block attempts to reload? > > + // Other location operations simply block use of JavaScript URLs cross origin. > > Maybe it has to do with re-POSTing form data? The other location operations can only generate GET requests. Interesting point.
Committed r74800: <http://trac.webkit.org/changeset/74800>