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

Description Joseph Pecoraro 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.
Comment 1 Radar WebKit Bug Importer 2014-01-21 23:48:20 PST
<rdar://problem/15878387>
Comment 2 Joseph Pecoraro 2014-01-21 23:52:13 PST
Created attachment 221834 [details]
[PATCH] Proposed Fix
Comment 3 Timothy Hatcher 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();
Comment 4 Timothy Hatcher 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.
Comment 5 Joseph Pecoraro 2014-01-22 10:35:27 PST
Oops yes, I forgot to git add that part.
Comment 6 Joseph Pecoraro 2014-01-22 10:53:26 PST
Landed <http://trac.webkit.org/changeset/162534>.
Comment 7 Joseph Pecoraro 2014-01-22 12:58:42 PST
Rolled this out in <http://trac.webkit.org/changeset/162550> because it broke tests. Investigating.
Comment 8 Joseph Pecoraro 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.
Comment 9 Joseph Pecoraro 2014-01-22 17:59:02 PST
Created attachment 221931 [details]
[PATCH] Proposed Fix - Passing Tests

Hmm, rebased.
Comment 10 Joseph Pecoraro 2014-01-22 18:12:53 PST
Well, this is unfortunate. Why don't the patches apply? =/
Comment 11 Joseph Pecoraro 2014-01-22 18:19:51 PST
Created attachment 221932 [details]
[PATCH] Proposed Fix - Passing Tests

Again, now that the bug was reopened.
Comment 12 WebKit Commit Bot 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.
Comment 13 Joseph Pecoraro 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.
Comment 14 Geoffrey Garen 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.
Comment 15 Joseph Pecoraro 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"?
Comment 16 Geoffrey Garen 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.
Comment 17 Timothy Hatcher 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?
Comment 18 Timothy Hatcher 2014-01-23 13:01:04 PST
Or InspectedTargetDestroyed.
Comment 19 Joseph Pecoraro 2014-01-23 13:23:55 PST
(In reply to comment #18)
> Or InspectedTargetDestroyed.

Target > Object > Context.

I'll go with InspectedTargetDestroyed.
Comment 20 Geoffrey Garen 2014-01-23 14:39:49 PST
Nice!
Comment 21 Joseph Pecoraro 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.
Comment 22 WebKit Commit Bot 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.
Comment 23 Joseph Pecoraro 2014-01-23 16:01:08 PST
Created attachment 222037 [details]
[PATCH] For Bots 2

With Windows Symbol.
Comment 24 Joseph Pecoraro 2014-01-23 16:06:23 PST
Created attachment 222038 [details]
[PATCH] For Bots 3
Comment 25 WebKit Commit Bot 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.
Comment 26 Joseph Pecoraro 2014-01-23 17:24:50 PST
Boom! Windows built. I'll land.
Comment 27 Joseph Pecoraro 2014-01-23 18:34:04 PST
Landed <http://trac.webkit.org/changeset/162676>.
Comment 28 Simon Fraser (smfr) 2014-01-23 22:47:59 PST
This seems to have broken 42 tests:
http://build.webkit.org/results/Apple%20Mavericks%20Debug%20WK1%20(Tests)/r162690%20(2192)/results.html
Comment 29 Joseph Pecoraro 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.
Comment 30 Joseph Pecoraro 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.
Comment 31 Joseph Pecoraro 2014-01-24 10:40:11 PST
Opened: <https://webkit.org/b/127566> fast/profiler tests ASSERTing after moving recompileAllJSFunctions off a timer