Bug 127409

Summary: Web Inspector: Remove recompileAllJSFunctions timer in ScriptDebugServer
Product: WebKit Reporter: Joseph Pecoraro <joepeck>
Component: Web InspectorAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: bunhere, cdumez, commit-queue, dbates, ggaren, graouts, gyuyoung.kim, joepeck, mark.lam, simon.fraser, timothy, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
[PATCH] Proposed Fix
timothy: review+, timothy: commit-queue-
[PATCH] Proposed Fix - Passing Tests
none
[PATCH] Proposed Fix - Passing Tests
none
[PATCH] Proposed Fix - Passing Tests
ggaren: review+
[PATCH] For Bots 1
none
[PATCH] For Bots 2
none
[PATCH] For Bots 3 none

Joseph Pecoraro
Reported 2014-01-21 23:48:10 PST
The m_recompileTimer recompileAllJSFunctions after a delay timer is no longer needed. Any path reaching recompileAllJSFunctionsSoon was not inside of a JS stack, the vm.entryScope is always empty. If we ever do hit this again there is an ASSERT inside of JSC::Debugger that would catch this. Now that we don't delay the call to recompileAllJSFunctions, be sure to set the Debugger on the Page/JSGlobalObjects before calling recompileAllJSFunctions in order to get the Debugger sourceParsed callbacks.
Attachments
[PATCH] Proposed Fix (9.37 KB, patch)
2014-01-21 23:52 PST, Joseph Pecoraro
timothy: review+
timothy: commit-queue-
[PATCH] Proposed Fix - Passing Tests (116.08 KB, patch)
2014-01-22 17:45 PST, Joseph Pecoraro
no flags
[PATCH] Proposed Fix - Passing Tests (120.53 KB, patch)
2014-01-22 17:59 PST, Joseph Pecoraro
no flags
[PATCH] Proposed Fix - Passing Tests (120.53 KB, patch)
2014-01-22 18:19 PST, Joseph Pecoraro
ggaren: review+
[PATCH] For Bots 1 (121.80 KB, patch)
2014-01-23 15:12 PST, Joseph Pecoraro
no flags
[PATCH] For Bots 2 (130.93 KB, patch)
2014-01-23 16:01 PST, Joseph Pecoraro
no flags
[PATCH] For Bots 3 (122.02 KB, patch)
2014-01-23 16:06 PST, Joseph Pecoraro
no flags
Radar WebKit Bug Importer
Comment 1 2014-01-21 23:48:20 PST
Joseph Pecoraro
Comment 2 2014-01-21 23:52:13 PST
Created attachment 221834 [details] [PATCH] Proposed Fix
Timothy Hatcher
Comment 3 2014-01-22 09:39:50 PST
/Volumes/Data/EWS/WebKit/Source/WebCore/bindings/js/ScriptDebugServer.cpp:306:5: error: use of undeclared identifier 'TimerBase' TimerBase::fireTimersInNestedEventLoop();
Timothy Hatcher
Comment 4 2014-01-22 09:40:31 PST
Comment on attachment 221834 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=221834&action=review > Source/WebCore/bindings/js/ScriptDebugServer.h:-37 > -#include "Timer.h" I think you need to keep the include or move it to the C++ file to fix the build.
Joseph Pecoraro
Comment 5 2014-01-22 10:35:27 PST
Oops yes, I forgot to git add that part.
Joseph Pecoraro
Comment 6 2014-01-22 10:53:26 PST
Joseph Pecoraro
Comment 7 2014-01-22 12:58:42 PST
Rolled this out in <http://trac.webkit.org/changeset/162550> because it broke tests. Investigating.
Joseph Pecoraro
Comment 8 2014-01-22 17:45:32 PST
Created attachment 221927 [details] [PATCH] Proposed Fix - Passing Tests Problem: When the Web Inspector was implicitly closed via JavaScript (e.g. window.close() or in LayoutTests as window.internals.closeDummyInspectorFrontend()) we were disconnecting InspectorDebuggerAgent and calling to recompileAllJSFunctions from within a JavaScript stack. Solution: Give agents a reason enum when they are disconnecting. The reasons can be: - InspectedObjectIsBeingDestroyed - no need to recompile functions here, the object is going to be deleted. - FrontendDisconnected - we should recompile functions so the page can be as good as new Other Stuff: Now that we don't use a Timer for recompiling scripts, as soon as the backend gets DebuggerAgent.enable, it starts recompiling and sending sourceParsed events to the frontend. That changes the order expected by inspector-protocol tests. Not in any significant way, but it makes the setBreakpointsActive calls output seemingly random. Remove all these unnecessary calls and output of "Breakpoints Enabled". Breakpoints are already enabled by default anyways.
Joseph Pecoraro
Comment 9 2014-01-22 17:59:02 PST
Created attachment 221931 [details] [PATCH] Proposed Fix - Passing Tests Hmm, rebased.
Joseph Pecoraro
Comment 10 2014-01-22 18:12:53 PST
Well, this is unfortunate. Why don't the patches apply? =/
Joseph Pecoraro
Comment 11 2014-01-22 18:19:51 PST
Created attachment 221932 [details] [PATCH] Proposed Fix - Passing Tests Again, now that the bug was reopened.
WebKit Commit Bot
Comment 12 2014-01-22 18:21:45 PST
Attachment 221932 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/runtime/JSGlobalObjectDebuggable.h:59: The parameter name "reason" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/WebKit2/WebProcess/WebPage/WebInspector.cpp:41: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/JavaScriptCore/inspector/agents/InspectorAgent.h:54: The parameter name "reason" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/inspector/InspectorAgentBase.h:47: The parameter name "reason" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/inspector/JSGlobalObjectInspectorController.h:56: The parameter name "reason" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/WebCore/ChangeLog:8: You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible. [changelog/nonewtests] [5] ERROR: Source/JavaScriptCore/inspector/InspectorAgentRegistry.h:46: The parameter name "reason" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 7 in 87 files If any of these errors are false positives, please file a bug against check-webkit-style.
Joseph Pecoraro
Comment 13 2014-01-22 19:03:09 PST
> Creating library C:\cygwin\home\buildbot\WebKit\WebKitBuild\Release\lib32\WebKit.lib and object C:\cygwin\home\buildbot\WebKit\WebKitBuild\Release\lib32\WebKit.exp > 1>WebKit.exp : error LNK2001: unresolved external symbol "public: void __thiscall WebCore::InspectorController::disconnectFrontend(void)" (?disconnectFrontend@InspectorController@WebCore@@QAEXXZ) > 1>C:\cygwin\home\buildbot\WebKit\WebKitBuild\Release\bin32\WebKit.dll : fatal error LNK1120: 1 unresolved externals > 1>Done Building Project "C:\cygwin\home\buildbot\WebKit\Source\WebKit\WebKit.vcxproj\WebKit\WebKit.vcxproj" (Build target(s)) -- FAILED. Symbol changed name because it takes a parameter now.
Geoffrey Garen
Comment 14 2014-01-22 20:27:50 PST
Comment on attachment 221932 [details] [PATCH] Proposed Fix - Passing Tests View in context: https://bugs.webkit.org/attachment.cgi?id=221932&action=review r=me > Source/JavaScriptCore/inspector/InspectorAgentBase.h:38 > + InspectedObjectBeingDestroyed, Can we be a little more specific here? Based on your comment below, how about "InspectedPageClosed"? > Source/WebCore/inspector/InspectorDebuggerAgent.cpp:127 > + disable(false); Let's put "false" in a named local variable, to give context to its meaning.
Joseph Pecoraro
Comment 15 2014-01-23 11:43:38 PST
(In reply to comment #14) > (From update of attachment 221932 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=221932&action=review > > r=me > > > Source/JavaScriptCore/inspector/InspectorAgentBase.h:38 > > + InspectedObjectBeingDestroyed, > > Can we be a little more specific here? Based on your comment below, how about "InspectedPageClosed"? If you're debugging a JSContext/JSContextRef this would mean the JSGlobalObject is being destroyed. If you're debugging a WebCore::Page, this would mean the WebCore::Page is closing. I think "InspectedObject..." better satisfies both of these and potential future inspector debuggable targets. I'm open to other naming suggestions. Would you prefer "InspectedObjectClosed"?
Geoffrey Garen
Comment 16 2014-01-23 12:25:47 PST
> If you're debugging a JSContext/JSContextRef this would mean the JSGlobalObject is being destroyed. > If you're debugging a WebCore::Page, this would mean the WebCore::Page is closing. Got it. > I think "InspectedObject..." better satisfies both of these and potential future inspector debuggable targets. > > I'm open to other naming suggestions. Would you prefer "InspectedObjectClosed"? Hmmm... no, I guess "closed" is not great, since, in the JSContext case you pointed out, nothing got "closed". I guess what we're trying track is which side will be destroyed. If the inspector will be destroyed, it needs to put the inspected thingy back to normal; if the inspected thingy will be destroyed, well, that's the end of that, and there's nothing to put back to normal. How about: InspectorDestroyed InspectedObjectDestroyed Even though "object" is still a bit abstract, hopefully similar phrasing will highlight the contrast between "Inspector" and "InspectedObject". Anyway, I'll leave the final decision up to you.
Timothy Hatcher
Comment 17 2014-01-23 13:00:39 PST
> How about: > InspectorDestroyed > InspectedObjectDestroyed > > Even though "object" is still a bit abstract, hopefully similar phrasing will highlight the contrast between "Inspector" and "InspectedObject". I like those. What about InspectedContextDestroyed instead of InspectedObjectDestroyed?
Timothy Hatcher
Comment 18 2014-01-23 13:01:04 PST
Or InspectedTargetDestroyed.
Joseph Pecoraro
Comment 19 2014-01-23 13:23:55 PST
(In reply to comment #18) > Or InspectedTargetDestroyed. Target > Object > Context. I'll go with InspectedTargetDestroyed.
Geoffrey Garen
Comment 20 2014-01-23 14:39:49 PST
Nice!
Joseph Pecoraro
Comment 21 2014-01-23 15:12:33 PST
Created attachment 222031 [details] [PATCH] For Bots 1 This patch removes the windows export for the old symbol. Need the bot to tell me the new symbol name.
WebKit Commit Bot
Comment 22 2014-01-23 15:15:12 PST
Attachment 222031 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/runtime/JSGlobalObjectDebuggable.h:59: The parameter name "reason" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/WebKit2/WebProcess/WebPage/WebInspector.cpp:41: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/JavaScriptCore/inspector/agents/InspectorAgent.h:54: The parameter name "reason" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/inspector/InspectorAgentBase.h:47: The parameter name "reason" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/inspector/JSGlobalObjectInspectorController.h:56: The parameter name "reason" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/WebCore/ChangeLog:8: You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible. [changelog/nonewtests] [5] ERROR: Source/JavaScriptCore/inspector/InspectorAgentRegistry.h:46: The parameter name "reason" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 7 in 87 files If any of these errors are false positives, please file a bug against check-webkit-style.
Joseph Pecoraro
Comment 23 2014-01-23 16:01:08 PST
Created attachment 222037 [details] [PATCH] For Bots 2 With Windows Symbol.
Joseph Pecoraro
Comment 24 2014-01-23 16:06:23 PST
Created attachment 222038 [details] [PATCH] For Bots 3
WebKit Commit Bot
Comment 25 2014-01-23 16:08:20 PST
Attachment 222038 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/runtime/JSGlobalObjectDebuggable.h:59: The parameter name "reason" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/WebKit2/WebProcess/WebPage/WebInspector.cpp:41: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/JavaScriptCore/inspector/agents/InspectorAgent.h:54: The parameter name "reason" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/inspector/InspectorAgentBase.h:47: The parameter name "reason" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/inspector/JSGlobalObjectInspectorController.h:56: The parameter name "reason" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/WebCore/ChangeLog:8: You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible. [changelog/nonewtests] [5] ERROR: Source/JavaScriptCore/inspector/InspectorAgentRegistry.h:46: The parameter name "reason" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 7 in 88 files If any of these errors are false positives, please file a bug against check-webkit-style.
Joseph Pecoraro
Comment 26 2014-01-23 17:24:50 PST
Boom! Windows built. I'll land.
Joseph Pecoraro
Comment 27 2014-01-23 18:34:04 PST
Simon Fraser (smfr)
Comment 28 2014-01-23 22:47:59 PST
Joseph Pecoraro
Comment 29 2014-01-23 23:20:06 PST
(In reply to comment #28) > This seems to have broken 42 tests: > http://build.webkit.org/results/Apple%20Mavericks%20Debug%20WK1%20(Tests)/r162690%20(2192)/results.html Bah, I'll look into this tomorrow. Thanks for pointing it out.
Joseph Pecoraro
Comment 30 2014-01-24 10:38:05 PST
Another internal method causing recompilation within a JS stack: window.internals.setJavaScriptProfilingEnabled(bool) This Enables/Disables the Profiler which now recompiles synchronously within a JS stack. I can add a timer here in window.internals and do this after a loop. But its looking more and more like it might be better if the VM handled recompiling when appropriate instead of us trying to avoid calling it within a JS stack.
Joseph Pecoraro
Comment 31 2014-01-24 10:40:11 PST
Opened: <https://webkit.org/b/127566> fast/profiler tests ASSERTing after moving recompileAllJSFunctions off a timer
Note You need to log in before you can comment on or make changes to this bug.