Bug 201201 – REGRESSION (r249078): Flaky crash in com.apple.JavaScriptCore: Inspector::InjectedScriptModule::ensureInjected

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Ryan Haddad]

Created attachment 377379 [details]
Crash log

CRASHING TEST: inspector/heap/getRemoteObject.html

Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   com.apple.JavaScriptCore      	0x000000010f9b8db3 WTFCrashWithInfo(int, char const*, char const*, int) + 19 (Assertions.h:616)
1   com.apple.JavaScriptCore      	0x00000001101d8456 Inspector::InjectedScriptModule::ensureInjected(Inspector::InjectedScriptManager*, Inspector::InjectedScript const&) + 1110
2   com.apple.WebCore             	0x000000010c228c50 WebCore::CommandLineAPIModule::injectIfNeeded(Inspector::InjectedScriptManager*, Inspector::InjectedScript const&) + 112 (CommandLineAPIModule.cpp:44)
3   com.apple.JavaScriptCore      	0x00000001101d7217 Inspector::InjectedScriptManager::injectedScriptFor(JSC::ExecState*) + 679 (InjectedScriptBase.h:50)
4   com.apple.JavaScriptCore      	0x0000000110256ab3 Inspector::InspectorHeapAgent::getRemoteObject(WTF::String&, int, WTF::String const*, WTF::RefPtr<Inspector::Protocol::Runtime::RemoteObject, WTF::DumbPtrTraits<Inspector::Protocol::Runtime::RemoteObject> >&) + 179 (InspectorHeapAgent.cpp:251)
5   com.apple.JavaScriptCore      	0x000000011020122c Inspector::HeapBackendDispatcher::getRemoteObject(long, WTF::RefPtr<WTF::JSONImpl::Object, WTF::DumbPtrTraits<WTF::JSONImpl::Object> >&&) + 860 (InspectorBackendDispatchers.cpp:4014)
6   com.apple.JavaScriptCore      	0x00000001101ffd53 Inspector::HeapBackendDispatcher::dispatch(long, WTF::String const&, WTF::Ref<WTF::JSONImpl::Object, WTF::DumbPtrTraits<WTF::JSONImpl::Object> >&&) + 1059 (utility:918)
7   com.apple.JavaScriptCore      	0x00000001101d9fdd Inspector::BackendDispatcher::dispatch(WTF::String const&) + 2349 (Ref.h:59)
8   com.apple.WebKit              	0x0000000109cc90d3 WebKit::WebPage::didReceiveWebPageMessage(IPC::Connection&, IPC::Decoder&) + 5555 (WebPageMessageReceiver.cpp:1024)
9   com.apple.WebKit              	0x000000010983d7ae IPC::MessageReceiverMap::dispatchMessage(IPC::Connection&, IPC::Decoder&) + 126
10  com.apple.WebKit              	0x0000000109bd03f6 WebKit::WebProcess::didReceiveMessage(IPC::Connection&, IPC::Decoder&) + 28 (WebProcess.cpp:726)
11  com.apple.WebKit              	0x0000000109828d22 IPC::Connection::dispatchMessage(std::__1::unique_ptr<IPC::Decoder, std::__1::default_delete<IPC::Decoder> >) + 158 (Connection.cpp:993)
12  com.apple.WebKit              	0x000000010982c114 IPC::Connection::dispatchOneIncomingMessage() + 190 (Connection.cpp:1060)
13  com.apple.JavaScriptCore      	0x000000010f9ec8c4 WTF::RunLoop::performWork() + 228 (RunLoop.cpp:107)
14  com.apple.JavaScriptCore      	0x000000010f9ecb52 WTF::RunLoop::performWork(void*) + 34 (RunLoopCF.cpp:39)
15  com.apple.CoreFoundation      	0x00007fff41e9c683 __CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION__ + 17
16  com.apple.CoreFoundation      	0x00007fff41e9c629 __CFRunLoopDoSource0 + 108
17  com.apple.CoreFoundation      	0x00007fff41e7ffeb __CFRunLoopDoSources0 + 195
18  com.apple.CoreFoundation      	0x00007fff41e7f5b5 __CFRunLoopRun + 1189
19  com.apple.CoreFoundation      	0x00007fff41e7eebe CFRunLoopRunSpecific + 455
20  com.apple.Foundation          	0x00007fff440e37df -[NSRunLoop(NSRunLoop) runMode:beforeDate:] + 280
21  com.apple.Foundation          	0x00007fff440e36b4 -[NSRunLoop(NSRunLoop) run] + 76
22  libxpc.dylib                  	0x00007fff6e00e077 _xpc_objc_main + 552
23  libxpc.dylib                  	0x00007fff6e00db79 xpc_main + 433
24  com.apple.WebKit              	0x00000001099874bb WebKit::XPCServiceMain(int, char const**) + 547
25  libdyld.dylib                 	0x00007fff6ddd53d5 start + 1



https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=inspector%2Fheap%2FgetRemoteObject.html

[Ryan Haddad]

stderr:

Failed to parse/execute CommandLineAPI!
//# sourceURL=__InjectedScript_CommandLineAPIModuleSource.js?(function(InjectedScriptHost,inspectedGlobalObject,injectedScriptId,injectedScript,RemoteObject,CommandLineAPIHost){injectedScript._inspectObject=function(object){if(arguments.length===0)?return;let objectId=RemoteObject.create(object,"");let hints={};switch(RemoteObject.describe(object)){case"Database":let databaseId=CommandLineAPIHost.databaseId(object)?if(databaseId)?hints.databaseId=databaseId;break;case"Storage":let storageId=CommandLineAPIHost.storageId(object)?if(storageId)?hints.domStorageId=InjectedScriptHost.evaluate("("+storageId+")");break;}?CommandLineAPIHost.inspect(objectId,hints);return object;};injectedScript.addCommandLineAPIGetter("0",function(){return CommandLineAPIHost.inspectedObject();});injectedScript.addCommandLineAPIMethod("copy",function(object){let string=null;let subtype=RemoteObject.subtype(object);if(subtype==="node")?string=object.outerHTML;else if(subtype==="regexp")?string=""+object;else if(injectedScript.isPrimitiveValue(object))?string=""+object;else if(typeof object==="symbol")?string=String(object);else if(typeof object==="function")?string=""+object;else{try{string=JSON.stringify(object,null,"  ");}catch{string=""+object;}}?CommandLineAPIHost.copyText(string);});injectedScript.addCommandLineAPIMethod("getEventListeners",function(target){return CommandLineAPIHost.getEventListeners(target);});function normalizeEventTypes(types){if(types===undefined)?types=["mouse","key","touch","control","abort","blur","change","devicemotion","deviceorientation","error","focus","load","reset","resize","scroll","search","select","submit","unload"];else if(typeof types==="string")?types=[types];let result=[];for(let i=0;i<types.length;i++){if(types[i]==="mouse")?result.push("click","dblclick","mousedown","mousemove","mouseout","mouseover","mouseup","mousewheel");else if(types[i]==="key")?result.push("keydown","keypress","keyup","textInput");else if(types[i]==="touch")?result.push("touchcancel","touchend","touchmove","touchstart");else if(types[i]==="control")?result.push("blur","change","focus","reset","resize","scroll","select","submit","zoom");else?result.push(types[i]);}?return result;}?function logEvent(event)?{inspectedGlobalObject.console.log(event.type,event);}?injectedScript.addCommandLineAPIMethod("monitorEvents",function(object,types){if(!object||!object.addEventListener||!object.removeEventListener)?return;types=normalizeEventTypes(types);for(let i=0;i<types.length;++i){object.removeEventListener(types[i],logEvent,false);object.addEventListener(types[i],logEvent,false);}});injectedScript.addCommandLineAPIMethod("unmonitorEvents",function(object,types){if(!object||!object.addEventListener||!object.removeEventListener)?return;types=normalizeEventTypes(types);for(let i=0;i<types.length;++i)?object.removeEventListener(types[i],logEvent,false);});if(inspectedGlobalObject.document&&inspectedGlobalObject.Node){function canQuerySelectorOnNode(node){return node&&InjectedScriptHost.subtype(node)==="node"&&(node.nodeType===inspectedGlobalObject.Node.ELEMENT_NODE||node.nodeType===inspectedGlobalObject.Node.DOCUMENT_NODE||node.nodeType===inspectedGlobalObject.Node.DOCUMENT_FRAGMENT_NODE);}?injectedScript.addCommandLineAPIMethod("$",function(selector,start){if(canQuerySelectorOnNode(start))?return start.querySelector(selector);let result=inspectedGlobalObject.document.querySelector(selector);if(result)?return result;if(selector&&selector[0]!=="#"){result=inspectedGlobalObject.document.getElementById(selector);if(result){inspectedGlobalObject.console.warn("The console function $() has changed from $=getElementById(id) to $=querySelector(selector). You might try $(\"#%s\")",selector);return null;}}?return result;});injectedScript.addCommandLineAPIMethod("$$",function(selector,start){if(canQuerySelectorOnNode(start))?return Array.from(start.querySelectorAll(selector));return Array.from(inspectedGlobalObject.document.querySelectorAll(selector));});injectedScript.addCommandLineAPIMethod("$x",function(xpath,context){let doc=(context&&context.ownerDocument)||inspectedGlobalObject.document;var result=doc.evaluate(xpath,context||doc,null,XPathResult.ANY_TYPE,null);switch(result.resultType){case XPathResult.NUMBER_TYPE:return result.numberValue;case XPathResult.STRING_TYPE:return result.stringValue;case XPathResult.BOOLEAN_TYPE:return result.booleanValue;default:var nodes=[];var node;while(node=result.iterateNext())?nodes.push(node);return nodes;}});}})?

[Ryan Haddad]

I think this is related to https://trac.webkit.org/changeset/249078

[Radar WebKit Bug Importer]

<rdar://problem/54771560>

[Devin Rousso]

Created attachment 377758 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 377758 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=377758&action=review

Seems fine. I actually liked the old methods better, but I don't have a strong preference.

> Source/JavaScriptCore/inspector/InjectedScriptModule.cpp:75
> +        hadException = false;

This shouldn't be necessary.

> Source/JavaScriptCore/inspector/InjectedScriptSource.js:-436
> -    setInspectObject(callback)
> -    {
> -        this._inspectObject = callback;
> -    }
> -
> -    addCommandLineAPIGetter(name, func)
> -    {
> -        InjectedScript.CommandLineAPI._getters.push({name, func});
> -    }
> -
> -    addCommandLineAPIMethod(name, func)
> -    {
> -        func.toString = function() { return "function " + name + "() { [Command Line API] }" };
> -        InjectedScript.CommandLineAPI._methods.push({name, func});
> -    }

I found this approach really clean!

[Devin Rousso]

Comment on attachment 377758 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=377758&action=review

>> Source/JavaScriptCore/inspector/InjectedScriptModule.cpp:75
>> +        hadException = false;
> 
> This shouldn't be necessary.

There's nothing that resets `hadException` back to `false` in the case that it was `true` for the first `callFunctionWithEvalEnabled`.  If `hasInjectedModule` threw an exception (`hadException = true`) and the resulting `injectModule` did NOT throw an exception, `hadException` would still be `true`.

>> Source/JavaScriptCore/inspector/InjectedScriptSource.js:-436
>> -    }
> 
> I found this approach really clean!

I'm not as much of a fan of this approach as it's somewhat of a weird paradigm (calling a function on an instance of a class that adds values to a static list).  I'd have preferred to have exposed the `InjectedScript` class itself and have these be static, but at that point we should just expose the `CommandLineAPI` class and have additional callers add to it directly.

[Devin Rousso]

Created attachment 377921 [details]
Patch

Update expected output of additional tests.

[WebKit Commit Bot]

Comment on attachment 377921 [details]
Patch

Clearing flags on attachment: 377921

Committed r249445: <https://trac.webkit.org/changeset/249445>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.