Bug 144636

Summary:

Web Inspector: Crash under WebCore::domWindowFromExecState reloading page with inspector open

Product:

WebKit

Reporter:

Joseph Pecoraro <joepeck>

Component:

Web Inspector

Assignee:

Joseph Pecoraro <joepeck>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

ap, commit-queue, graouts, joepeck, jonowells, mark.lam, mattbaker, nvasilyev, timothy, webkit-bug-importer

Priority:

Keywords:

DoNotImportToRadar, InRadar

Version:

528+ (Nightly build)

Hardware:

All

OS:

All

Attachments:

Description	Flags
[PATCH] Proposed Fix	none

Description Joseph Pecoraro 2015-05-05 13:35:53 PDT

* SUMMARY
Crash under WebCore::domWindowFromExecState reloading page with inspector open.

* STEPS TO REPRODUCE:
1. Open SkyDrive live.com Excel spreadsheet
2. Open Inspector
3. Cmd+R to reload
  => likely crash

* CRASH
Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   com.apple.JavaScriptCore      	0x000000011024260a WTFCrash + 42 (Assertions.cpp:321)
1   com.apple.WebCore             	0x0000000113e4c7e1 JSC::asObject(JSC::JSCell*) + 65 (JSObject.h:1190)
2   com.apple.WebCore             	0x0000000113e4c790 JSC::asObject(JSC::JSValue) + 32 (JSObject.h:1196)
3   com.apple.WebCore             	0x0000000113e4cb95 JSC::Register::function() const + 85 (JSObject.h:1473)
4   com.apple.WebCore             	0x0000000113e4cafc JSC::ExecState::callee() const + 28 (CallFrame.h:46)
5   com.apple.WebCore             	0x0000000113f41f75 JSC::ExecState::lexicalGlobalObject() const + 21 (JSScope.h:244)
6   com.apple.WebCore             	0x00000001157de565 WebCore::domWindowFromExecState(JSC::ExecState*) + 21 (ScriptState.cpp:52)
7   com.apple.WebCore             	0x0000000115cfa608 WebCore::WebInjectedScriptManager::discardInjectedScriptsFor(WebCore::DOMWindow*) + 664 (WebInjectedScriptManager.cpp:76)
8   com.apple.WebCore             	0x0000000115cc8613 WebCore::WebConsoleAgent::frameWindowDiscarded(WebCore::DOMWindow*) + 259 (WebConsoleAgent.cpp:68)
9   com.apple.WebCore             	0x0000000114a83938 WebCore::InspectorInstrumentation::frameWindowDiscardedImpl(WebCore::InstrumentingAgents&, WebCore::DOMWindow*) + 56 (InspectorInstrumentation.cpp:191)
10  com.apple.WebCore             	0x00000001143e4d58 WebCore::InspectorInstrumentation::frameWindowDiscarded(WebCore::Frame*, WebCore::DOMWindow*) + 56 (InspectorInstrumentation.h:547)
11  com.apple.WebCore             	0x0000000114615ee4 WebCore::FrameLoader::clear(WebCore::Document*, bool, bool, bool) + 324 (FrameLoader.cpp:628)
12  com.apple.WebCore             	0x0000000114347252 WebCore::DocumentWriter::begin(WebCore::URL const&, bool, WebCore::Document*) + 498 (DocumentWriter.cpp:142)

Comment 1 Joseph Pecoraro 2015-05-05 13:35:59 PDT

Here we are in frameWindowDiscarded, trying to remove certain (globalExec) ExecStates from our table, but the JSGlobalObject that ExecState is tied to was already destroyed.

Comment 2 Joseph Pecoraro 2015-05-05 13:37:11 PDT

<rdar://problem/15811895>

Comment 3 Joseph Pecoraro 2015-05-05 14:17:24 PDT

Created attachment 252405 [details]
[PATCH] Proposed Fix

I was unable to create a reduction/regression test for this, but seeing as this was caught by running tests in the past I think that is fine.

In general we should cleanup our management of InjectedScript/ExecutionContextIdentifiers instead of always making an InjectedScript like we do here. I'd like to do that cleanup in a separate change and address the crash now.

Comment 4 Mark Lam 2015-05-05 15:17:49 PDT

Comment on attachment 252405 [details]
[PATCH] Proposed Fix

r=me

Comment 5 WebKit Commit Bot 2015-05-05 16:09:31 PDT

Comment on attachment 252405 [details]
[PATCH] Proposed Fix

Clearing flags on attachment: 252405

Committed r183838: <http://trac.webkit.org/changeset/183838>

Comment 6 WebKit Commit Bot 2015-05-05 16:09:35 PDT

All reviewed patches have been landed.  Closing bug.

Comment 7 Alexey Proskuryakov 2015-05-05 20:03:31 PDT

> this was caught by running tests in the past I think that is fine

Should any test expectations be updated now? This was seen on tests, but only because the crash was misattributed sometimes.

Looking at the fix, I'm puzzled about why this is Yosemite only.

# Also, these tests are flaky in Debug/Release builds, <https://bugs.webkit.org/show_bug.cgi?id=138636>
# and <https://bugs.webkit.org/show_bug.cgi?id=129817>.
[ Yosemite+ ] inspector/css/matched-style-properties.html [ Pass Timeout ]
[ Yosemite+ ] inspector/css/pseudo-element-matches.html [ Pass Timeout ]
[ Yosemite+ ] inspector/css/selector-specificity.html [ Pass Timeout Crash ]
[ Yosemite+ ] inspector/dom/content-flow-content-removal.html [ Skip ]
[ Yosemite+ ] inspector/dom/content-flow-list.html [ Skip ]
[ Yosemite+ ] inspector/model/parse-script-syntax-tree.html [ Pass Timeout ]
[ Yosemite+ ] inspector/test-harness-trivially-works.html [ Skip ]