Bug 41350 – [v8] Web Inspector: inspected page crashes on attempt to change iframe's src attribute

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector (Deprecated)

[Yury Semikhatsky]

Inspected page crashes on attempt to change iframe's src attribute.

Steps to reproduce:

1. Open a document with iframe.
2. Open DevTools and try to change the iframe's "src" attribute value.

Result:
Inspected renderer crashes with the following stack trace:
0023eec8 60641af2 chrome_60120000!v8::Object::GetPointerFromInternalField
0023eef4 60641b0a chrome_60120000!WebCore::V8Proxy::retrieveWindow+0x37
0023ef0c 60641b42 chrome_60120000!WebCore::V8Proxy::retrieveFrame+0x10
0023ef18 6056208a chrome_60120000!WebCore::V8Proxy::retrieveFrameForEnteredContext+0x19
0023ef54 6053f249 chrome_60120000!WebCore::ScriptController::processingUserGesture+0x11
0023ef60 605b970c chrome_60120000!WebCore::FrameLoader::isProcessingUserGesture+0x2d
0023f068 6067b061 chrome_60120000!WebCore::SubframeLoader::requestFrame+0xcb
0023f088 6067b3b6 chrome_60120000!WebCore::HTMLFrameElementBase::openURL+0x6d
0023f0a4 6067b0a3 chrome_60120000!WebCore::HTMLFrameElementBase::setLocation+0x58
0023f0b8 60b25c47 chrome_60120000!WebCore::HTMLFrameElementBase::parseMappedAttribute+0x2a
0023f0d0 6066f739 chrome_60120000!WebCore::HTMLIFrameElement::parseMappedAttribute+0x108
0023f0f4 6057a843 chrome_60120000!WebCore::StyledElement::attributeChanged+0xd0
0023f118 60685d17 chrome_60120000!WebCore::Element::setAttribute+0x117
0023f138 60ad99c6 chrome_60120000!WebCore::InspectorDOMAgent::setAttribute+0x4e
0023f168 60c05767 chrome_60120000!WebCore::InspectorBackendInternal::setAttributeCallback+0xc2
0023f1c4 60c05a6f chrome_60120000!v8::internal::HandleApiCallHelper<0>+0x167
0023f2b8 60be2048 chrome_60120000!v8::internal::Builtin_HandleApiCall+0xf
0023f2fc 60be2116 chrome_60120000!v8::internal::Invoke+0xc8
0023f320 60bb24d2 chrome_60120000!v8::internal::Execution::Call+0x26
0023f368 60785cd7 chrome_60120000!v8::Function::Call+0xc2
0023f3d4 60774d91 chrome_60120000!WebKit::DebuggerAgentImpl::executeUtilityFunction+0x18c
0023f400 60773cbc chrome_60120000!WebKit::WebDevToolsAgentImpl::dispatchOnInspectorController+0x28
0023f444 60774e09 chrome_60120000!WebKit::ToolsAgentDispatch::dispatch+0x135
0023f454 6022f5ae chrome_60120000!WebKit::WebDevToolsAgentImpl::dispatchMessageFromFrontend+0x18
0023f47c 602300b6 chrome_60120000!DevToolsAgent::OnRpcMessage+0x2d
0023f4f4 6022f22b chrome_60120000!IPC::MessageWithTuple<Tuple1<DevToolsMessageData> >::Dispatch<DevToolsAgent,void (__thiscall DevToolsAgent::*)(DevToolsMessageData const &)>+0x39
0023f514 6020c0c3 chrome_60120000!DevToolsAgent::OnMessageReceived+0x74
0023f5f8 604c769f chrome_60120000!RenderView::OnMessageReceived+0x9c
0023f604 604c7679 chrome_60120000!MessageRouter::RouteMessage+0x23
0023f60c 604c63d7 chrome_60120000!MessageRouter::OnMessageReceived+0x1b
0023f62c 603ae13b chrome_60120000!ChildThread::OnMessageReceived+0x6f
0023f638 601dc74f chrome_60120000!RunnableMethod<DetectTabLanguageFunction,void (__thiscall DetectTabLanguageFunction::*)(std::basic_string<char,std::char_traits<char>,std::allocator<char> > const &),Tuple1<std::basic_string<char,std::char_traits<char>,std::allocator<char> > > >::Run+0x17
0023f668 601dc7db chrome_60120000!MessageLoop::RunTask+0x97
0023f678 601dc971 chrome_60120000!MessageLoop::DeferOrRunPendingTask+0x28
0023f6a8 601ed398 chrome_60120000!MessageLoop::DoWork+0x71
0023f6d4 601dc589 chrome_60120000!base::MessagePumpDefault::Run+0xb9
0023f6e0 601dc50e chrome_60120000!MessageLoop::RunInternal+0x31
0023f6e8 601dc4bc chrome_60120000!MessageLoop::RunHandler+0x17
0023f708 601fff89 chrome_60120000!MessageLoop::Run+0x15
0023f970 60123b98 chrome_60120000!RendererMain+0x289
0023fb2c 011f3892 chrome_60120000!ChromeMain+0x67d
0023fba8 011f532e chrome!MainDllLoader::Launch+0xf3
0023fc08 01236d91 chrome!wWinMain+0xdd
0023fc98 755b1174 chrome!__tmainCRTStartup+0x112
0023fca4 76f4b3f5 kernel32!BaseThreadInitThunk+0xe
0023fce4 76f4b3c8 ntdll!__RtlUserThreadStart+0x70
0023fcfc 00000000 ntdll!_RtlUserThreadStart+0x1b

[Yury Semikhatsky]

Corresponding Chromium bug: http://code.google.com/p/chromium/issues/detail?id=44070

[Yury Semikhatsky]

Created attachment 60025 [details]
Patch

[Adam Barth]

Comment on attachment 60025 [details]
Patch

WebCore/bindings/v8/ScriptController.cpp:165
 +      v8::Handle<v8::Context> v8Context = m_proxy->mainWorldContext();
Are you sure it's ok to switch to using this frame's context instead of the entered frame?  What if the event is triggered in the other frame?  It might be missing from this frame's global object and we'd incorrectly think that there wasn't a user gesture.

http://trac.webkit.org/browser/trunk/WebCore/bindings/js/ScriptController.cpp#L247

I guess the caller of this API typically finds the entered Frame before calling this method.  It's probably ok, but I'm slightly worried about regressions.

Can you add a test for the global->Get => global->GetHiddenValue change?  I think the way to do that is to steal the event from a real user gesture and then assign it to window.event during a non-user gesture.

[Yury Semikhatsky]

Created attachment 60118 [details]
Patch

[Yury Semikhatsky]

(In reply to comment #3)
> (From update of attachment 60025 [details])
> WebCore/bindings/v8/ScriptController.cpp:165
>  +      v8::Handle<v8::Context> v8Context = m_proxy->mainWorldContext();
> Are you sure it's ok to switch to using this frame's context instead of the entered frame?  What if the event is triggered in the other frame?  It might be missing from this frame's global object and we'd incorrectly think that there wasn't a user gesture.
>
If we want to retrieve the frame from the stack, this method shouldn't be instance method on ScriptController as it gets all the data from static functions. As you said the contract should be that caller finds Frame/ScriptController before calling the method.

 
> http://trac.webkit.org/browser/trunk/WebCore/bindings/js/ScriptController.cpp#L247
> 
> I guess the caller of this API typically finds the entered Frame before calling this method.  It's probably ok, but I'm slightly worried about regressions.
> 
As you pointed out the changed code matches JSC's behavior. So it should work correctly.


> Can you add a test for the global->Get => global->GetHiddenValue change?  I think the way to do that is to steal the event from a real user gesture and then assign it to window.event during a non-user gesture.

Added a test for this.

[Adam Barth]

Comment on attachment 60118 [details]
Patch

Thanks for the test.  Hopefully all the callers follow the correct convention.  You might want to look though them to be sure.

[Yury Semikhatsky]

Comment on attachment 60118 [details]
Patch

Clearing flags on attachment: 60118

Committed r62246: <http://trac.webkit.org/changeset/62246>

[Yury Semikhatsky]

All reviewed patches have been landed.  Closing bug.