Bug 31293 – Web Inspector: Enable 'console.profile()' and 'console.profileEnd()' regardless of JAVASCRIPT_DEBUGGER

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

[Mikhail Naganov]

Currently 'console.profile()' and 'console.profileEnd()' are only available when building against JSC. I want to make them available regardless of engine being used.

[Mikhail Naganov]

Created attachment 42864 [details]
proposed change

[Mikhail Naganov]

Created attachment 42866 [details]
proposed change

Fixed a small issue

[Timothy Hatcher]

Comment on attachment 42866 [details]
proposed change

> +#else
> +    bool profilerEnabled() const { return enabled(); }
>  #endif

This should likely return false, sicne there is no profiler.


>  #if ENABLE(JAVASCRIPT_DEBUGGER)
>      typedef HashMap<unsigned int, RefPtr<JSC::Profile> > ProfilesMap;
>  
>      void startUserInitiatedProfilingSoon();
> -    void toggleRecordButton(bool);
>      void enableDebuggerFromFrontend(bool always);
>      void getProfileHeaders(long callId);
>      void getProfile(long callId, unsigned uid);

Why isn't everything profile related moved out of the if ENABLE(JAVASCRIPT_DEBUGGER) blocks? (Not just this file, other places too.)

[Mikhail Naganov]

(In reply to comment #3)
> (From update of attachment 42866 [details])
> > +#else
> > +    bool profilerEnabled() const { return enabled(); }
> >  #endif
> 
> This should likely return false, sicne there is no profiler.
> 

No. Since in this case '{start,stop}UserInitiatedProfiling' will never run.

> 
> >  #if ENABLE(JAVASCRIPT_DEBUGGER)
> >      typedef HashMap<unsigned int, RefPtr<JSC::Profile> > ProfilesMap;
> >  
> >      void startUserInitiatedProfilingSoon();
> > -    void toggleRecordButton(bool);
> >      void enableDebuggerFromFrontend(bool always);
> >      void getProfileHeaders(long callId);
> >      void getProfile(long callId, unsigned uid);
> 
> Why isn't everything profile related moved out of the if
> ENABLE(JAVASCRIPT_DEBUGGER) blocks? (Not just this file, other places too.)

I've tried to make more functions to be available, but this requires more additional changes making patch even more complicated, because there are things like 'toJS', 'toJSDOMWindow', which also need to be done in engine-agnostic way.

As for the 'console.profiles' property, I'm a bit confused, because it doesn't conform to injected script approach at all---it needs to be re-done in 'getProfiles' / 'didGetProfiles' async way, but this breaks the "standard" interface of the 'console' object. This is a problem, actually.

So I would like not to submit this patch as is, and continue with profiling interface generalization in small steps. Also, mind that some of these changes will become two-sided for DevTools, so it's better to keep them simple.

[Timothy Hatcher]

What do you mean by "two-sided for DevTools"?

console.profiles is used by layout tests, not by the Inspector.

[Mikhail Naganov]

(In reply to comment #5)
> What do you mean by "two-sided for DevTools"?
>

We call "two-sided" patches that consist of a patch to WebKit and a patch to Chromium. The main hassle is that a patch to WK Inspector can break DevTools because it changes some interface, so it may require a "preventive" patch into Chromium which ensures that on WebKit rollout we will not break, and possibly a "cleanup" patch later.

As an example, if I make "InspectorController.profilerEnabled" and friends available regardless of JAVASCRIPT_DEBUGGER, it will break DevTools' assumption that profiler is always enabled, so proper adjustments will be needed to made in our code.

This is the main motivating factor for upstreaming stuff into WebKit. 
 
> console.profiles is used by layout tests, not by the Inspector.

Ah, yes, sorry. I forgot that console is actually accessed from a page, and commands are only passed from Inspector console in order to be evaluated in page's context. So making them accessible will not cause a trouble, right.

But can we make them always available as a next step? Because this requires V8 binding for ScriptProfile to be something more that a simple stub.

[Pavel Feldman]

Comment on attachment 42866 [details]
proposed change

We agreed on a simple approach to this. Clearing review flag.

[Mikhail Naganov]

Created attachment 43100 [details]
another approach

Pavel, Timothy,

I found that much easier way is to fork 'profile' and 'profileEnd' methods of console. How do you like it?

[Timothy Hatcher]

Comment on attachment 43100 [details]
another approach

This seems fine to me.

[WebKit Commit Bot]

Comment on attachment 43100 [details]
another approach

Rejecting patch 43100 from commit-queue.

Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--quiet', '--exit-after-n-failures=1']" exit_code: 1
Running build-dumprendertree
Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests
Testing 11616 test cases.
inspector/console-dir.html -> crashed

Exiting early after 1 failures. 9311 tests run.
490.22s total testing time

9310 test cases (99%) succeeded
1 test case (<1%) crashed
5 test cases (<1%) had stderr output

[Adam Barth]

Comment on attachment 43100 [details]
another approach

Crash looks like it might be real.

[Pavel Feldman]

Comment on attachment 43100 [details]
another approach

> +JSValue JSConsole::profile(ExecState* exec, const ArgList& args)
> +{
> +    ScriptCallStack callStack(exec, args, 1);
> +    impl()->profile(args.at(0).getString(), &callStack);
> +    return jsUndefined();
> +}

I don't think that test crash is relevant, but I think you should have borrowed all the logic from the generated bindings:

ScriptCallStack callStack(exec, args, 1);
const UString& title = valueToStringWithUndefinedOrNullCheck(exec, args.at(0));
imp->profile(title, &callStack);

Btw, why do v8 customs not use profile names?

[Brian Weinstein]

I've seen console-dir and console-dirxml crash on my machine (on Windows, using run-webkit-tests or just straight DumpRenderTree). I'm on a different one right now, but can post a backtrace to this or a different bug if there is one tracking.

[Mikhail Naganov]

Created attachment 43146 [details]
fixed argument conversion in JSC binding

I think 'console dir' test crash is completely unrelated. I ran 'inspector' tests for several times w/o any troubles.

I fixed arguments conversion in JSC binding. As for V8, it currently doesn't afford an ability to tag profiles with custom labels.

[WebKit Commit Bot]

Comment on attachment 43146 [details]
fixed argument conversion in JSC binding

Rejecting patch 43146 from commit-queue.

Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--quiet', '--exit-after-n-failures=1']" exit_code: 1
Running build-dumprendertree
Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests
Testing 11618 test cases.
http/tests/xmlhttprequest/abort-crash.html -> crashed

Exiting early after 1 failures. 9146 tests run.
436.30s total testing time

9145 test cases (99%) succeeded
1 test case (<1%) crashed
5 test cases (<1%) had stderr output

[Pavel Feldman]

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	WebCore/ChangeLog
	M	WebCore/WebCore.gypi
	M	WebCore/bindings/js/JSConsoleCustom.cpp
	A	WebCore/bindings/v8/custom/V8ConsoleCustom.cpp
	M	WebCore/bindings/v8/custom/V8CustomBinding.h
	M	WebCore/page/Console.idl
Committed r50933

[Eric Seidel (no email)]

Filed bug 31480 about the crash.

[Laszlo Gombos]

This change broke the JSC builds (e.g. QtWebKit) with JAVASCRIPT_DEBUGGER turned off (this was a supported configuration).

Building QtWebKit with --minimal will result in the following linker error:

WebKitBuildQt/Release/lib/libQtWebKit.so: undefined reference to
`WebCore::JSConsole::profile(JSC::ExecState*, JSC::ArgList const&)'
WebKitBuildQt/Release/lib/libQtWebKit.so: undefined reference to
`WebCore::JSConsole::profileEnd(JSC::ExecState*, JSC::ArgList const&)'


I'm not sure how to resolve this problem.
 - we could either change the semantics of JAVASCRIPT_DEBUGGER so that it only disables the debugger and not the profiler (myself would not prefer this option)
 - revert this change and Chromium can maybe define JAVASCRIPT_DEBUGGER (instead of removing the flags). This is probably not an option either as Chromium will not build in this configuration. 

Maybe we should use the CHROMIUM test for this cases instead of changing the meaning of the JAVASCRIPT_DEBUGGER flag.

[Mikhail Naganov]

I think, I can make more fine-grained #ifdefs in JSConsoleCustom, so functions will be there, but they will be empty.

The only drawback is that 'console.profile' and 'console.profileEnd' will present even if 'JAVASCRIPT_DEBUGGER' is turned off. But I think this isn't a problem.

[Laszlo Gombos]

I would change the test in Console.idl to 
#if defined(ENABLE_JAVASCRIPT_DEBUGGER) && ENABLE_JAVASCRIPT_DEBUGGER || (defined(PLATFORM_CHROMIUM))

and revert the change made in WebCore/bindings/js/JSConsoleCustom.cpp.

[Laszlo Gombos]

We should either REOPEN this bug (I do not seems to have permission to do so) or use this new bug (https://bugs.webkit.org/show_bug.cgi?id=31575) to track this work.

[Mikhail Naganov]

Laszlo, your proposal for introducing platform check makes sense. And it will actually suffice---no need to change JSConsoleCustom, because:
 - in case of JSC w/debugger there will be both declaration and definition;
 - in case of JSC w/o debugger there will be no declaration and no definition;
 - in case of Chromium, there will be declaration and definition in V8 bindings (JSC bindings will be ignored);

I will make a patch and test it with Chromium, could you then test it with Qt (as I heard building Qt version of WK is tricky due to a need of bleeding edge glibc)?