Bug 127409 – Web Inspector: Remove recompileAllJSFunctions timer in ScriptDebugServer

• Component: Web Inspector
• Status: RESOLVED FIXED

[Joseph Pecoraro]

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.

[Radar WebKit Bug Importer]

<rdar://problem/15878387>

[Joseph Pecoraro]

Created attachment 221834 [details]
[PATCH] Proposed Fix

[Timothy Hatcher]

/Volumes/Data/EWS/WebKit/Source/WebCore/bindings/js/ScriptDebugServer.cpp:306:5: error: use of undeclared identifier 'TimerBase'
    TimerBase::fireTimersInNestedEventLoop();

[Timothy Hatcher]

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]

Oops yes, I forgot to git add that part.

[Joseph Pecoraro]

Landed <http://trac.webkit.org/changeset/162534>.

[Joseph Pecoraro]

Rolled this out in <http://trac.webkit.org/changeset/162550> because it broke tests. Investigating.

[Joseph Pecoraro]

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]

Created attachment 221931 [details]
[PATCH] Proposed Fix - Passing Tests

Hmm, rebased.

[Joseph Pecoraro]

Well, this is unfortunate. Why don't the patches apply? =/

[Joseph Pecoraro]

Created attachment 221932 [details]
[PATCH] Proposed Fix - Passing Tests

Again, now that the bug was reopened.

[WebKit Commit Bot]

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]

>            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 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]

(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]

> 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]

> 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]

Or InspectedTargetDestroyed.

[Joseph Pecoraro]

(In reply to comment #18)
> Or InspectedTargetDestroyed.

Target > Object > Context.

I'll go with InspectedTargetDestroyed.

[Geoffrey Garen]

Nice!

[Joseph Pecoraro]

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]

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]

Created attachment 222037 [details]
[PATCH] For Bots 2

With Windows Symbol.

[Joseph Pecoraro]

Created attachment 222038 [details]
[PATCH] For Bots 3

[WebKit Commit Bot]

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]

Boom! Windows built. I'll land.

[Joseph Pecoraro]

Landed <http://trac.webkit.org/changeset/162676>.

[Simon Fraser (smfr)]

This seems to have broken 42 tests:
http://build.webkit.org/results/Apple%20Mavericks%20Debug%20WK1%20(Tests)/r162690%20(2192)/results.html

[Joseph Pecoraro]

(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]

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]

Opened: <https://webkit.org/b/127566> fast/profiler tests ASSERTing after moving recompileAllJSFunctions off a timer