Web Inspector: the inspector should live in its own process

• Component: Web Inspector
• Status: RESOLVED FIXED

[Brian Burg]

It would be approximately way easier to profile if it were isolated from the inspected page's Web Content process.

[Radar WebKit Bug Importer]

<rdar://problem/17748414>

[Timothy Hatcher]

Internal dupe of <rdar://problem/11749409>.

[Timothy Hatcher]

I am working on this.

[Timothy Hatcher]

Created attachment 237676 [details]
WIP

[Timothy Hatcher]

Created attachment 237677 [details]
WIP (correct)

[Timothy Hatcher]

Created attachment 237727 [details]
WIP (latest)

[Brian Burg]

Comment on attachment 237727 [details]
WIP (latest)

View in context: https://bugs.webkit.org/attachment.cgi?id=237727&action=review

Looks pretty slick overall. Seeking some clarification on the code path for GTK/EFL: does it change at all? Some of the messages looked deleted but I didn't see any updates to the other platform-specific files.

> Source/WebCore/inspector/InspectorController.cpp:198
> +void InspectorController::setInspectorFrontendClient(InspectorFrontendClient* inspectorFrontendClient)

Before, the Controller owned the client. Who owns it now? Can this pass a reference?

> Source/WebKit/mac/WebCoreSupport/WebInspectorClient.mm:-137
> -    , m_frontendClient(0)

This should still be initialized to nullptr, right?

> Source/WebKit2/UIProcess/WebInspectorProxy.cpp:110
>          pageGroup->preferences().setLogsPageMessagesToSystemConsoleEnabled(true);

Can you update the comment above this to mean "developers can inspector the inspector in debug mode _without changing any settings_" ? AFAICT, that's what it does.

> Source/WebKit2/WebProcess/WebPage/WebInspector.cpp:1
>  /*

Per the naming conventions, files that implement the InspectorProcess probably should be in Source/WebKit2/InspectorProcess/* not in WebProcess.

> Source/WebKit2/WebProcess/WebPage/WebInspectorUI.cpp:30
> +#include "WebPage.h"

newline after ENABLE(...)

[Timothy Hatcher]

Comment on attachment 237727 [details]
WIP (latest)

View in context: https://bugs.webkit.org/attachment.cgi?id=237727&action=review

>> Source/WebKit/mac/WebCoreSupport/WebInspectorClient.mm:-137
>> -    , m_frontendClient(0)
> 
> This should still be initialized to nullptr, right?

It was removed. They aren't in the same process anymore.

>> Source/WebKit2/UIProcess/WebInspectorProxy.cpp:110
>>          pageGroup->preferences().setLogsPageMessagesToSystemConsoleEnabled(true);
> 
> Can you update the comment above this to mean "developers can inspector the inspector in debug mode _without changing any settings_" ? AFAICT, that's what it does.

Sure.

>> Source/WebKit2/WebProcess/WebPage/WebInspector.cpp:1
>>  /*
> 
> Per the naming conventions, files that implement the InspectorProcess probably should be in Source/WebKit2/InspectorProcess/* not in WebProcess.

They are in WebProcess, just two different web processes now.

[Timothy Hatcher]

Comment on attachment 237727 [details]
WIP (latest)

View in context: https://bugs.webkit.org/attachment.cgi?id=237727&action=review

>>> Source/WebKit/mac/WebCoreSupport/WebInspectorClient.mm:-137
>>> -    , m_frontendClient(0)
>> 
>> This should still be initialized to nullptr, right?
> 
> It was removed. They aren't in the same process anymore.

Oh, this is WK1. m_frontendClient is a unique_ptr now which defaults to nullptr.

[Timothy Hatcher]

Comment on attachment 237727 [details]
WIP (latest)

View in context: https://bugs.webkit.org/attachment.cgi?id=237727&action=review

>> Source/WebCore/inspector/InspectorController.cpp:198
>> +void InspectorController::setInspectorFrontendClient(InspectorFrontendClient* inspectorFrontendClient)
> 
> Before, the Controller owned the client. Who owns it now? Can this pass a reference?

The higher level objects in WK2 and WK1 own it now with a lifetime of the Page. It might be possible to pass as a reference, but it needs to be nullable.

[Timothy Hatcher]

Created attachment 237798 [details]
WIP (almost final)

Posting to let EWS try things while I look into a layout test hang.

[WebKit Commit Bot]

Attachment 237798 [details] did not pass style-queue:


ERROR: Source/WebKit2/WebProcess/WebPage/WebInspectorUI.cpp:32:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebKit2/WebProcess/WebPage/WebInspectorUI.cpp:39:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebKit2/WebProcess/WebPage/WebInspectorUI.cpp:42:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebKit2/WebProcess/WebPage/WebInspectorUI.cpp:145:  Declaration has space between type name and * in const char *sideString  [whitespace/declaration] [3]
ERROR: Source/WebKit2/WebProcess/WebPage/WebInspectorUI.h:39:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/WebCore/WebCore.exp.in:0:  Source/WebCore/WebCore.exp.in should be sorted, use Tools/Scripts/sort-export-file script  [list/order] [5]
Total errors found: 6 in 38 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Timothy Hatcher]

Created attachment 237803 [details]
WIP (almost final x2)

[Timothy Hatcher]

Created attachment 237806 [details]
WIP (almost final x3)

[Timothy Hatcher]

Created attachment 237818 [details]
WIP (almost final x4)

[Build Bot]

Comment on attachment 237818 [details]
WIP (almost final x4)

Attachment 237818 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5085997704413184

New failing tests:
http/tests/xmlhttprequest/access-control-repeated-failed-preflight-crash.html

[Build Bot]

Created attachment 237830 [details]
Archive of layout-test-results from webkit-ews-08 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-08  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Timothy Hatcher]

Created attachment 238037 [details]
WIP (latest final)

[Brian Burg]

Comment on attachment 238037 [details]
WIP (latest final)

View in context: https://bugs.webkit.org/attachment.cgi?id=238037&action=review

Will look at this more later, but some quick comments:

* Missing ChangeLogs.
* Many tests in LayoutTests/inspector/ will crash under WK1 if you force them to run. This is a regression.
* Saving resources seems to work, but it names the downloaded files as "Unknown". Is this a regression?

> Source/WebKit2/UIProcess/gtk/WebInspectorProxyGtk.cpp:185
> +    // Set a default sizes based on InspectorFrontendClientLocal.

grammar.

[Brian Burg]

It would also be nice to name the inspector process something different from com.apple.WebKit.WebContent[.Development] so that different web processes can be easily distinguished from Xcode.

[Brian Burg]

Looks like Safari expects some symbols that were removed in this patch. Maybe these should be stubbed out and deprecated until Safari no longer uses them.

dyld: lazy symbol binding failed: Symbol not found: _WKInspectorIsProfilingJavaScript
  Referenced from: /System/Library/StagedFrameworks/Safari/Safari.framework/Versions/A/Safari
  Expected in: /Users/burg/repos/replay-staging/OpenSource/WebKitBuild/Debug/WebKit2.framework/Versions/A/WebKit2

[Brian Burg]

(In reply to comment #21)
> Looks like Safari expects some symbols that were removed in this patch. Maybe these should be stubbed out and deprecated until Safari no longer uses them.
> 
> dyld: lazy symbol binding failed: Symbol not found: _WKInspectorIsProfilingJavaScript
>   Referenced from: /System/Library/StagedFrameworks/Safari/Safari.framework/Versions/A/Safari
>   Expected in: /Users/burg/repos/replay-staging/OpenSource/WebKitBuild/Debug/WebKit2.framework/Versions/A/WebKit2

To clarify, this happens when you open the Develop menu in Safari launched from `run-safari`.

Another bug (which I'll have to unapply the patch to get around, for now): the `pagehide` event is not getting fired when the inspector closes, so breakpoints and the current view state are not being persisted across reopening the inspector.

[Timothy Hatcher]

(In reply to comment #20)
> It would also be nice to name the inspector process something different from com.apple.WebKit.WebContent[.Development] so that different web processes can be easily distinguished from Xcode.

Joe and I talked about this too. One issue is we are not guaranteed to be in our own process. Safari sets the process limit to 20. We should force our own process that all inspectors share, then we can name it properly.

[Timothy Hatcher]

(In reply to comment #21)
> Looks like Safari expects some symbols that were removed in this patch. Maybe these should be stubbed out and deprecated until Safari no longer uses them.
> 
> dyld: lazy symbol binding failed: Symbol not found: _WKInspectorIsProfilingJavaScript
>   Referenced from: /System/Library/StagedFrameworks/Safari/Safari.framework/Versions/A/Safari
>   Expected in: /Users/burg/repos/replay-staging/OpenSource/WebKitBuild/Debug/WebKit2.framework/Versions/A/WebKit2

Oops. Yeah, we need to stub those out.

[Timothy Hatcher]

(In reply to comment #22)
> Another bug (which I'll have to unapply the patch to get around, for now): the `pagehide` event is not getting fired when the inspector closes, so breakpoints and the current view state are not being persisted across reopening the inspector.

I thought Joe was having issues with page hide not firing already?

[Brian Burg]

(In reply to comment #25)
> (In reply to comment #22)
> > Another bug (which I'll have to unapply the patch to get around, for now): the `pagehide` event is not getting fired when the inspector closes, so breakpoints and the current view state are not being persisted across reopening the inspector.
> 
> I thought Joe was having issues with page hide not firing already?

I recall some discussion about this, but can't find a bug. And, it is definitely a regression with this patch applied vs not.

[Joseph Pecoraro]

(In reply to comment #26)
> (In reply to comment #25)
> > (In reply to comment #22)
> > > Another bug (which I'll have to unapply the patch to get around, for now): the `pagehide` event is not getting fired when the inspector closes, so breakpoints and the current view state are not being persisted across reopening the inspector.
> > 
> > I thought Joe was having issues with page hide not firing already?
> 
> I recall some discussion about this, but can't find a bug. And, it is definitely a regression with this patch applied vs not.

In my last tip of tree build page hide events were working as expected. Were you still seeing issues? That will certainly affect state restoration as well.

[Timothy Hatcher]

Created attachment 238427 [details]
WIP (For Bots)

This version passes inspector and inspector-protocol tests. Still need ChangeLogs, then I will post it for review. Checking EWS first.

[Brian Burg]

Comment on attachment 238427 [details]
WIP (For Bots)

View in context: https://bugs.webkit.org/attachment.cgi?id=238427&action=review

> Source/WebInspectorUI/UserInterface/Protocol/InspectorFrontendAPI.js:127
> +        console.assert(InspectorFrontendAPI[methodName], "Unexpected InspectorFrontendAPI method name: " + InspectorFrontendAPI[methodName]);

I wish we could do:

if (!console.assert(pred, ...))
    return;

But this would require some more intelligent rewriting to strip console assertions.

[Timothy Hatcher]

Created attachment 238430 [details]
WIP (Rebased For Bots)

[Brian Burg]

Comment on attachment 238430 [details]
WIP (Rebased For Bots)

"Binary files a/Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js and b/Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js differ"

^-- this is causing the patch to not apply. What diff command are you using? I've never had this problem using `Tools/Scripts/webkit-patch upload`

[Timothy Hatcher]

Created attachment 238437 [details]
WIP (Binary Patch)

[WebKit Commit Bot]

Attachment 238437 [details] did not pass style-queue:


ERROR: Source/WebKit2/UIProcess/WebInspectorProxy.cpp:351:  Missing space before {  [whitespace/braces] [5]
Total errors found: 1 in 52 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Timothy Hatcher]

Created attachment 238446 [details]
Patch

[Timothy Hatcher]

This will need a WebKIt2 owner review.

[Brian Burg]

Comment on attachment 238446 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=238446&action=review

> Source/WebKit2/ChangeLog:108
> +        Stop taking an optional InspectorFrontendChannel and be it directly. Create a connection that can

'be it'

[Timothy Hatcher]

Created attachment 238454 [details]
Patch

[Timothy Hatcher]

Created attachment 238483 [details]
Patch

[Timothy Hatcher]

Created attachment 238499 [details]
Patch

[Timothy Hatcher]

Created attachment 238503 [details]
Patch

[Anders Carlsson]

Comment on attachment 238503 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=238503&action=review

> Source/WebCore/testing/Internals.cpp:1495
> +    m_frontendWindow.clear();

This should use = nullptr.

> Source/WebKit2/UIProcess/API/C/WKInspector.cpp:177
> -bool WKInspectorIsDebuggingJavaScript(WKInspectorRef inspectorRef)
> +bool WKInspectorIsDebuggingJavaScript(WKInspectorRef)

As discussed, these should move to WKDeprecatedFunctions.

> Source/WebKit2/UIProcess/WebInspectorProxy.cpp:359
> +    static std::once_flag onceFlag;
> +    static LazyNeverDestroyed<RefPtr<WebContext>> context;
> +
> +    std::call_once(onceFlag, [] {
> +        WebContextConfiguration configuration;
> +        WebContext::applyPlatformSpecificConfigurationDefaults(configuration);
> +        context.construct(WebContext::create(configuration));
> +        context.get()->setProcessModel(ProcessModelMultipleSecondaryProcesses);
> +    });

There's no need to put a smart pointer inside a LazyNeverDestroyed. This can just be something like:

static WebContext* context;
if (!context) {
 /// initialize context
}

return *context;

> Source/WebKit2/WebProcess/WebPage/WebInspector.messages.in:25
> -messages -> WebInspector LegacyReceiver {
> +messages -> WebInspector {

Very nice!

[Timothy Hatcher]

http://trac.webkit.org/changeset/173929

[Joseph Pecoraro]

32-bit mac build fix: <http://trac.webkit.org/changeset/173933>

[Timothy Hatcher]

WK1 test crash fix: http://trac.webkit.org/changeset/173936

[Brent Fulgham]

64-bit Windows build fix: http://trac.webkit.org/changeset/173986.