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.
<rdar://problem/15878387>
Created attachment 221834 [details] [PATCH] Proposed Fix
/Volumes/Data/EWS/WebKit/Source/WebCore/bindings/js/ScriptDebugServer.cpp:306:5: error: use of undeclared identifier 'TimerBase' TimerBase::fireTimersInNestedEventLoop();
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.
Oops yes, I forgot to git add that part.
Landed <http://trac.webkit.org/changeset/162534>.
Rolled this out in <http://trac.webkit.org/changeset/162550> because it broke tests. Investigating.
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.
Created attachment 221931 [details] [PATCH] Proposed Fix - Passing Tests Hmm, rebased.
Well, this is unfortunate. Why don't the patches apply? =/
Created attachment 221932 [details] [PATCH] Proposed Fix - Passing Tests Again, now that the bug was reopened.
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.
> 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.
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.
(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"?
> 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.
> 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?
Or InspectedTargetDestroyed.
(In reply to comment #18) > Or InspectedTargetDestroyed. Target > Object > Context. I'll go with InspectedTargetDestroyed.
Nice!
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.
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.
Created attachment 222037 [details] [PATCH] For Bots 2 With Windows Symbol.
Created attachment 222038 [details] [PATCH] For Bots 3
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.
Boom! Windows built. I'll land.
Landed <http://trac.webkit.org/changeset/162676>.
This seems to have broken 42 tests: http://build.webkit.org/results/Apple%20Mavericks%20Debug%20WK1%20(Tests)/r162690%20(2192)/results.html
(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.
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.
Opened: <https://webkit.org/b/127566> fast/profiler tests ASSERTing after moving recompileAllJSFunctions off a timer