Bug 28799 – REGRESSION(r47686-r47783): Debugger no longer works on reload

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

[Oliver Hunt]

The debugger no longer hits break points when a page is reloaded.

Steps to repro:
1. go to http://testsuites.opera.com/JSON/correctness/004.html
2. enable the debugger
3. set a breakpoint somewhere in the code of 004.js
4. reload the page

Result:
The existance of the breakpoints has no effect

Expected:
I expect us to have reached the breakpoint, the scope chain should be populated, etc

Obvious inspector patches that could have caused this:
r47717, r47724 -- seems to involve the debugger, handling displaying of scopes, etc
r47703 -- seems very likely to be at fault, seems like a massive change to the debugger
r47696, r47695 -- formatting issues, seem unlikely to be at fault

There are changes in JSC in this range as well, but they seem unlikely to have caused this kind of bustage, so i believe this is likely to be due to r47703

[Pavel Feldman]

This repeats for me on Safari 4.0.3 (5531.9). Why REGRESSION(r47686-r47783)?

[Pavel Feldman]

(In reply to comment #1)
> This repeats for me on Safari 4.0.3 (5531.9). Why REGRESSION(r47686-r47783)?

A simpler scenario does not work as of r47738 though (Oliver, you were reviewing it).

1. Open some page
2. Set a breakpoint in code that should execute in some event handler
3. Make execution hit the breakpoint (via clicking a button or something)

Expected: call frame and source line are highlighted
Actual: nothing is highlighted. Continue the execution, open inspector on
inspector, observe javascript error.


I've been reporting this as a follow up to https://bugs.webkit.org/show_bug.cgi?id=28691. The problem is that as of this change, when clicking a button, debugger stops on a breakpoint before the inline script snippet is reported as parsed.

[Oliver Hunt]

My bug explicitly did not state that source code was highlighted -- i am aware of the existing bug where source is not displayed -- the issue is that now you don't get a source list (in the past you did -- some sources were even available *boggle*), and you do not hit the breakpoint; JS runs to completion, the engine is never paused, and the scope information is never displayed.

[Pavel Feldman]

(In reply to comment #3)
> My bug explicitly did not state that source code was highlighted -- i am aware
> of the existing bug where source is not displayed -- the issue is that now you
> don't get a source list (in the past you did -- some sources were even
> available *boggle*), and you do not hit the breakpoint; JS runs to completion,
> the engine is never paused, and the scope information is never displayed.

I don't understand something. I am seeing the same on Safari 4.0.3. So I think it regressed earlier than 47686. Is that right?

[Oliver Hunt]

Ah ha, there had been a progression from 4.0.3 -> r47686 which has since regressed.

[Pavel Feldman]

I am seeing no debugger object available in the Parser::parse. I think the problem is that in r47738 that I am blaming exec->dynamicGlobalObject()->debugger() was replaced with exec->lexicalGlobalObject()->debugger(). Not sure what it is, could this be a problem?

[Gavin Barraclough]

It could well be. :-/

Previously the code was using the DGO to get the debugger when parsing, but the LGO during bytecode generation.  Since I was unifying the two stages to occur at the same time, this appeared to be incorrect.

I discussed this change with ggaren, and we decided to unify on using the LGO, which sounded more correct (our logic being that you care whether *this* frame is being debugged, not a caller frame).

[Geoffrey Garen]

I think we need to investigate why exec->lexicalGlobalObject()->debugger() is null even though the debugger is attached to the page.

[Oliver Hunt]

A big problem is that the debugger is broken prior to r44738 -- r47703 goes form a completely functional debugger (more so than i recall) to completely broken -- crashes at 
#0  0x00000001008ab224 in JSC::JSCell::isObject (this=0x0) at JSCell.h:140
#1  0x000000010088aac1 in JSC::asObject (value={m_ptr = 0x0}) at JSObject.h:255
#2  0x0000000100802425 in JSC::asFunction (value={m_ptr = 0x0}) at JSFunction.h:124
#3  0x000000010084cc7a in JSC::DebuggerCallFrame::calculatedFunctionName (this=0x11abe1508) at /Volumes/BigData/git/WebKit/OpenSource/JavaScriptCore/debugger/DebuggerCallFrame.cpp:55
#4  0x00000001011bb42f in WebCore::JavaScriptCallFrame::functionName (this=0x11abe1500) at /Volumes/BigData/git/WebKit/OpenSource/WebCore/inspector/JavaScriptCallFrame.cpp:78
#5  0x00000001012bf67c in WebCore::jsJavaScriptCallFrameFunctionName (exec=0x119115048, slot=@0x7fff5fbfadf0) at /Volumes/BigData/git/WebKit/OpenSource/WebKitBuild/Debug/DerivedSources/WebCore/JSJavaScriptCallFrame.cpp:153
#6  0x00000001011d146a in JSC::PropertySlot::getValue (this=0x7fff5fbfadf0, exec=0x119115048, propertyName=@0x1065a5bc0) at PropertySlot.h:62
#7  0x00000001012e144a in WebCore::JSQuarantinedObjectWrapper::getOwnPropertySlot (this=0x112be0900, exec=0x112c24260, identifier=@0x1065a5bc0, slot=@0x7fff5fbfaf30) at /Volumes/BigData/git/WebKit/OpenSource/WebCore/bindings/js/JSQuarantinedObjectWrapper.cpp:113
#8  0x000000010080c10b in JSC::JSCell::fastGetOwnPropertySlot (this=0x112be0900, exec=0x112c24260, propertyName=@0x1065a5bc0, slot=@0x7fff5fbfaf30) at JSObject.h:355
#9  0x00000001008344a8 in JSC::JSValue::get (this=0x7fff5fbfaf90, exec=0x112c24260, propertyName=@0x1065a5bc0, slot=@0x7fff5fbfaf30) at JSObject.h:580
#10 0x0000000100889ce7 in cti_op_get_by_id (args=0x7fff5fbfafd0) at /Volumes/BigData/git/WebKit/OpenSource/JavaScriptCore/jit/JITStubs.cpp:1197
#11 0x0000000100880d8f in WTF::doubleHash (key=Could not find the frame base for "WTF::doubleHash(unsigned int)".
) at HashTable.h:437
#12 0x0000000100866670 in JSC::JITCode::execute (this=0x11abe47e8, registerFile=0x106522c60, callFrame=0x112c24168, globalData=0x10595f400, exception=0x105960690) at JITCode.h:79
#13 0x0000000100853e42 in JSC::Interpreter::execute (this=0x106522c50, functionExecutable=0x11abe47d0, callFrame=0x1065e9da8, function=0x1124871c0, thisObj=0x111cd3240, args=@0x7fff5fbfb360, scopeChain=0x11abe3c20, exception=0x105960690) at /Volumes/BigData/git/WebKit/OpenSource/JavaScriptCore/interpreter/Interpreter.cpp:721
#14 0x00000001007b8cca in JSC::JSFunction::call (this=0x1124871c0, exec=0x1065e9da8, thisValue={m_ptr = 0x111cd3240}, args=@0x7fff5fbfb360) at JSFunction.cpp:120
#15 0x00000001007b8d7e in JSC::call (exec=0x1065e9da8, functionObject={m_ptr = 0x1124871c0}, callType=JSC::CallTypeJS, callData=@0x7fff5fbfb370, thisValue={m_ptr = 0x111cd3240}, args=@0x7fff5fbfb360) at CallData.cpp:39
#16 0x000000010155f9cd in WebCore::ScheduledAction::executeFunctionInContext (this=0x11abe3c90, globalObject=0x111cd32c0, thisValue={m_ptr = 0x111cd3240}) at /Volumes/BigData/git/WebKit/OpenSource/WebCore/bindings/js/ScheduledAction.cpp:105
#17 0x000000010155fcdf in WebCore::ScheduledAction::execute (this=0x11abe3c90, document=0x10592d600) at /Volumes/BigData/git/WebKit/OpenSource/WebCore/bindings/js/ScheduledAction.cpp:125
#18 0x000000010155fd9c in WebCore::ScheduledAction::execute (this=0x11abe3c90, context=0x10592d658) at /Volumes/BigData/git/WebKit/OpenSource/WebCore/bindings/js/ScheduledAction.cpp:76
#19 0x0000000100fefd56 in WebCore::DOMTimer::fired (this=0x11abe3cc0) at /Volumes/BigData/git/WebKit/OpenSource/WebCore/page/DOMTimer.cpp:124
#20 0x0000000101699206 in WebCore::ThreadTimers::fireTimers (this=0x104d34f10, fireTime=1251515228.074461, firingTimers=@0x7fff5fbfb540) at /Volumes/BigData/git/WebKit/OpenSource/WebCore/platform/ThreadTimers.cpp:111
#21 0x00000001016993e8 in WebCore::ThreadTimers::sharedTimerFiredInternal (this=0x104d34f10) at /Volumes/BigData/git/WebKit/OpenSource/WebCore/platform/ThreadTimers.cpp:141
#22 0x000000010169942d in WebCore::ThreadTimers::sharedTimerFired () at /Volumes/BigData/git/WebKit/OpenSource/WebCore/platform/ThreadTimers.cpp:122
#23 0x0000000101591168 in WebCore::timerFired () at /Volumes/BigData/git/WebKit/OpenSource/WebCore/platform/mac/SharedTimerMac.mm:86
#24 0x00007fff8482ea78 in __CFRunLoopRun ()
#25 0x00007fff8482d03f in CFRunLoopRunSpecific ()
#26 0x00007fff8383fc4e in RunCurrentEventLoopInMode ()
#27 0x00007fff8383fa53 in ReceiveNextEventCommon ()
#28 0x00007fff8383f90c in BlockUntilNextEventMatchingListInMode ()
#29 0x00007fff879f8570 in _DPSNextEvent ()
#30 0x00007fff879f7ed9 in -[NSApplication nextEventMatchingMask:untilDate:inMode:dequeue:] ()
#31 0x000000010000bcf0 in ?? ()
#32 0x0000000101034cd8 in WebCore::EventLoop::cycle (this=0x7fff5fbfc7de) at /Volumes/BigData/git/WebKit/OpenSource/WebCore/platform/mac/EventLoopMac.mm:35
#33 0x00000001011bbf0b in WebCore::JavaScriptDebugServer::pauseIfNeeded (this=0x104d35bd0, page=0x106152530) at /Volumes/BigData/git/WebKit/OpenSource/WebCore/inspector/JavaScriptDebugServer.cpp:440
#34 0x00000001011bc6bf in WebCore::JavaScriptDebugServer::atStatement (this=0x104d35bd0, debuggerCallFrame=@0x7fff5fbfc880, sourceID=4743601408, lineNumber=7) at /Volumes/BigData/git/WebKit/OpenSource/WebCore/inspector/JavaScriptDebugServer.cpp:468
#35 0x00000001008518f5 in JSC::Interpreter::debug (this=0x106522c50, callFrame=0x112c240a8, debugHookID=JSC::WillExecuteStatement, firstLine=7, lastLine=9) at /Volumes/BigData/git/WebKit/OpenSource/JavaScriptCore/interpreter/Interpreter.cpp:911
#36 0x0000000100881973 in cti_op_debug (args=0x7fff5fbfc930) at /Volumes/BigData/git/WebKit/OpenSource/JavaScriptCore/jit/JITStubs.cpp:3010
#37 0x0000000100880d8f in WTF::doubleHash (key=Could not find the frame base for "WTF::doubleHash(unsigned int)".
) at HashTable.h:437
#38 0x0000000100866670 in JSC::JITCode::execute (this=0x7fff5fbfcb68, registerFile=0x106522c60, callFrame=0x112c24048, globalData=0x10595f400, exception=0x7fff5fbfcc10) at JITCode.h:79
#39 0x000000010085422f in JSC::Interpreter::execute (this=0x106522c50, program=0x7fff5fbfcb50, callFrame=0x119115048, scopeChain=0x119115370, thisObj=0x111cd0000, exception=0x7fff5fbfcc10) at /Volumes/BigData/git/WebKit/OpenSource/JavaScriptCore/interpreter/Interpreter.cpp:655
#40 0x00000001007e981a in JSC::evaluate (exec=0x119115048, scopeChain=@0x119115000, source=@0x7fff5fbfd238, thisValue={m_ptr = 0x111cd0000}) at Completion.cpp:64
#41 0x00000001015636d7 in WebCore::ScriptController::evaluate (this=0x1050524a8, sourceCode=@0x7fff5fbfd230) at /Volumes/BigData/git/WebKit/OpenSource/WebCore/bindings/js/ScriptController.cpp:114
#42 0x0000000101079598 in WebCore::FrameLoader::executeScript (this=0x105052050, sourceCode=@0x7fff5fbfd230) at /Volumes/BigData/git/WebKit/OpenSource/WebCore/loader/FrameLoader.cpp:769
....

[Adam Roben (:aroben)]

<rdar://problem/7187624>

[Pavel Feldman]

Created attachment 38987 [details]
patch

[Timothy Hatcher]

Comment on attachment 38987 [details]
patch


> -            else if (!foundLocalScope && !localScope)
> +            else if (!foundLocalScope)

What does this fix/change?

[Pavel Feldman]

(In reply to comment #12)
> (From update of attachment 38987 [details])
> 
> > -            else if (!foundLocalScope && !localScope)
> > +            else if (!foundLocalScope)
> 
> What does this fix/change?

localScope is not defined here at all - it belongs to the UI part of this functionality. Fix actually makes refresh work fine. Function name fix was not enough...

[Oliver Hunt]

Comment on attachment 38987 [details]
patch

r=me

yay \o/

[Pavel Feldman]

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	WebCore/ChangeLog
	M	WebCore/inspector/front-end/InjectedScript.js
Committed r48024

[Eric Seidel (no email)]

Comment on attachment 38987 [details]
patch

Rejecting patch 38987 from commit-queue.  This patch will require manual commit.

Failed to run "['git', 'svn', 'dcommit']"  exit_code: 1  cwd: None

[Eric Seidel (no email)]

Comment on attachment 38987 [details]
patch

commit-queue race condition.  ChangeLog was out of date by the time the queue went to land it. :(

[Eric Seidel (no email)]

Comment on attachment 38987 [details]
patch

Actually, nevermind.  svn-apply could have trouble with this ChangeLog, so I would prefer someone land this manually.

[Eric Seidel (no email)]

Comment on attachment 38987 [details]
patch

Looks like this was already landed anyway. :)

[Adam Roben (:aroben)]

The bug still reproduces for me in ToT.

[Eric Seidel (no email)]

Comment on attachment 38987 [details]
patch

Obsoleting this patch since it appears it was already landed.

[Timothy Hatcher]

We should look at fixing this soonish.

[Pavel Feldman]

Getting back to a general issue where breakpoints are not hit in case they happen on load and/or in DOM0 handlers, I think the reason is that interaction between inspector controller and inspector front-end is now asynchronous.

Old control flow:
- Script is being parsed
- It is synchronously being pushed to the front-end
- All breakpoints are being synchronously set on source
- Execution continues, breakpoints are being hit

New control flow:
- Script is being parsed
- It is being asynchronously pushed to the front-end
- It continues execution, no breakpoints were set to hit
- Front-end sets breakpoints

There are two ways of fixing this:
1) Bring back synchronous operation
2) Store breakpoint in the inspector controller (InspectorBackend) and set them synchronously upon source parse.

I like 2 more because it reflects the remote debugging paradigm better. I am starting to work on this.

[Pavel Feldman]

Created attachment 49150 [details]
[PATCH] Proposed change.

Sorry about the big change, but it mostly contains mechanical changes described in the change log. It is just that I needed to persist breakpoints in InspectorController and decided to reuse JavaScriptDebugServer's BreakpointInfo. Then I saw that we have too much unnecessary complexity around if(USE_JSC) vs JavaScriptDebugServer in InspectorController/Backend/Frontend and moved server content to bindings.

This change does not fix the bug on its own. It makes debugger stop on breakpoints while loading page, but it does not show the source. Reason is that resource loading is not yet finished and it is still 'Other' viewer that has no source frame. I will follow up with another change within the same bug.

[WebKit Review Bot]

Attachment 49150 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/inspector/InspectorController.cpp:101:  "ScriptDebugServer.h" already included at WebCore/inspector/InspectorController.cpp:75  [build/include] [4]
Skipping input 'WebCore/inspector/JavaScriptDebugServer.cpp': Can't open for reading
Skipping input 'WebCore/inspector/JavaScriptDebugServer.h': Can't open for reading
Skipping input 'WebCore/inspector/JavaScriptDebugListener.h': Can't open for reading
WebCore/bindings/js/ScriptDebugServer.cpp:48:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 2 in 38 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Attachment 49150 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/293056

[Timothy Hatcher]

Comment on attachment 49150 [details]
[PATCH] Proposed change.

Some general comments:

1) It would be nice if common parts of ScriptDebugger.h were shared. This goes for other bindings/{js, v8} Script* files too. We should use an approach like the other platform files. Share a common header with common implementation and have platform specific implementations. That would be more maintainable than complete file forks.

2) JavaScriptDebugServer should have no dependancy on Inspector. So InspectorBreakpoint should be ScriptBreakpoint.

3) Like we talked about on IRC, I would like to see the Script* files moved to inspector/, and adopt the platform approch I mentioned in point 1.

This patch looks fine as long as you rename InspectorBreakpoint before landing.

[Pavel Feldman]

Created attachment 49164 [details]
[PATCH] Patch I'm going to land.

[Pavel Feldman]

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	R	WebCore/inspector/JavaScriptDebugListener.h => WebCore/inspector/ScriptBreakpoint.h
	D	WebCore/inspector/JavaScriptDebugServer.cpp
	D	WebCore/inspector/JavaScriptDebugServer.h
	M	WebCore/ChangeLog
	M	WebCore/GNUmakefile.am
	M	WebCore/WebCore.base.exp
	M	WebCore/WebCore.gypi
	M	WebCore/WebCore.order
	M	WebCore/WebCore.pro
	M	WebCore/WebCore.vcproj/WebCore.vcproj
	M	WebCore/WebCore.xcodeproj/project.pbxproj
	M	WebCore/bindings/js/JSInjectedScriptHostCustom.cpp
	M	WebCore/bindings/js/ScriptDebugServer.cpp
	M	WebCore/bindings/js/ScriptDebugServer.h
	M	WebCore/bindings/v8/ScriptDebugServer.cpp
	M	WebCore/bindings/v8/ScriptDebugServer.h
	M	WebCore/bindings/v8/custom/V8InjectedScriptHostCustom.cpp
	M	WebCore/inspector/InjectedScriptHost.cpp
	M	WebCore/inspector/InjectedScriptHost.h
	M	WebCore/inspector/InjectedScriptHost.idl
	M	WebCore/inspector/InspectorBackend.cpp
	M	WebCore/inspector/InspectorBackend.h
	M	WebCore/inspector/InspectorBackend.idl
	M	WebCore/inspector/InspectorController.cpp
	M	WebCore/inspector/InspectorController.h
	M	WebCore/inspector/InspectorFrontend.cpp
	M	WebCore/inspector/InspectorFrontend.h
	M	WebCore/inspector/front-end/Breakpoint.js
	M	WebCore/inspector/front-end/BreakpointsSidebarPane.js
	M	WebCore/inspector/front-end/InspectorBackendStub.js
	M	WebCore/inspector/front-end/ScriptsPanel.js
	M	WebCore/inspector/front-end/inspector.js
	M	WebCore/page/Page.cpp
	M	WebCore/platform/android/TemporaryLinkStubs.cpp
	M	WebKit/chromium/ChangeLog
	M	WebKit/chromium/src/js/DebuggerAgent.js
	M	WebKit/chromium/src/js/InspectorControllerImpl.js
Committed r55071

[Pavel Feldman]

Comment on attachment 49164 [details]
[PATCH] Patch I'm going to land.

Backend part landed, front-end to follow.

[Pavel Feldman]

Actually, it is now in the state it used to be in 4.0.4, so should not qualify as regression. Furthermore, it works fine on cached resources (will stop on breakpoint and show the source), but fails to work when scripts is parsed and stopped prior to the resource finished event.

I was thinking of a change that prevents scripts from being bound to resources prior to the resource's finished event and it works fine for secondary resources (external scripts). For main resource it would show multiple chunks of script tags first, and, once resource is loaded, it should replace chunks with the resource itself. Besides merging script entries upon resource finished event, there is a problem with chunks having 1-based lines numbering. It all breaks breakpoints.

I am thinking of closing this bug and opening another one that is not a regression that will address the problem above. It might need some non-trivial front-end work.

[Pavel Feldman]

New bug filed: https://bugs.webkit.org/show_bug.cgi?id=35232

[Pavel Feldman]

*** Bug 31043 has been marked as a duplicate of this bug. ***