Bug 166681 – WebInspector: refactor RemoteInspector to move cocoa specific code to their own files

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Carlos Garcia Campos]

Most of the RemoteInspector implementation is actually cross-platform, but RemoteInspector.mm and RemoteConnectionToTarget.mm need to be factored out.

[Carlos Garcia Campos]

Created attachment 298005 [details]
WIP patch

I need someone with a mac who can add make the XCode changes after the refactoring, please. I'll deal with builds breaks using EWS.

[Carlos Garcia Campos]

Created attachment 298008 [details]
WIP patch

[Carlos Garcia Campos]

Created attachment 298462 [details]
WIP: trying xcode changes

[WebKit Commit Bot]

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


ERROR: Source/JavaScriptCore/inspector/remote/cocoa/RemoteInspectorXPCConnection.mm:111:  No space between ^ and block definition.  [whitespace/brackets] [4]
ERROR: Source/JavaScriptCore/inspector/remote/cocoa/RemoteInspectorXPCConnection.mm:153:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/inspector/remote/cocoa/RemoteConnectionToTargetCocoa.mm:82:  No space between ^ and block definition.  [whitespace/brackets] [4]
ERROR: Source/JavaScriptCore/inspector/remote/cocoa/RemoteConnectionToTargetCocoa.mm:163:  No space between ^ and block definition.  [whitespace/brackets] [4]
ERROR: Source/JavaScriptCore/inspector/remote/cocoa/RemoteConnectionToTargetCocoa.mm:164:  No space between ^ and block definition.  [whitespace/brackets] [4]
ERROR: Source/JavaScriptCore/inspector/remote/cocoa/RemoteConnectionToTargetCocoa.mm:164:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/inspector/remote/cocoa/RemoteConnectionToTargetCocoa.mm:198:  No space between ^ and block definition.  [whitespace/brackets] [4]
ERROR: Source/JavaScriptCore/inspector/remote/cocoa/RemoteConnectionToTargetCocoa.mm:199:  No space between ^ and block definition.  [whitespace/brackets] [4]
ERROR: Source/JavaScriptCore/inspector/remote/cocoa/RemoteConnectionToTargetCocoa.mm:199:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/inspector/remote/cocoa/RemoteConnectionToTargetCocoa.mm:216:  No space between ^ and block definition.  [whitespace/brackets] [4]
ERROR: Source/JavaScriptCore/inspector/remote/cocoa/RemoteConnectionToTargetCocoa.mm:217:  No space between ^ and block definition.  [whitespace/brackets] [4]
ERROR: Source/JavaScriptCore/inspector/remote/cocoa/RemoteConnectionToTargetCocoa.mm:217:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/inspector/remote/cocoa/RemoteInspectorCocoa.mm:50:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
ERROR: Source/JavaScriptCore/inspector/remote/cocoa/RemoteInspectorCocoa.mm:69:  No space between ^ and block definition.  [whitespace/brackets] [4]
ERROR: Source/JavaScriptCore/inspector/remote/cocoa/RemoteInspectorCocoa.mm:71:  No space between ^ and block definition.  [whitespace/brackets] [4]
ERROR: Source/JavaScriptCore/inspector/remote/cocoa/RemoteInspectorCocoa.mm:147:  No space between ^ and block definition.  [whitespace/brackets] [4]
ERROR: Source/JavaScriptCore/inspector/remote/cocoa/RemoteInspectorCocoa.mm:209:  No space between ^ and block definition.  [whitespace/brackets] [4]
ERROR: Source/JavaScriptCore/inspector/remote/cocoa/RemoteInspectorCocoa.mm:452:  No space between ^ and block definition.  [whitespace/brackets] [4]
ERROR: Source/JavaScriptCore/inspector/remote/cocoa/RemoteInspectorCocoa.mm:562:  No space between ^ and block definition.  [whitespace/brackets] [4]
ERROR: Source/JavaScriptCore/inspector/remote/cocoa/RemoteInspectorCocoa.mm:588:  No space between ^ and block definition.  [whitespace/brackets] [4]
Total errors found: 20 in 13 files


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

[Carlos Garcia Campos]

Created attachment 298469 [details]
WIP: try to fix the mac builds and some style issues

[WebKit Commit Bot]

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


ERROR: Source/JavaScriptCore/inspector/remote/cocoa/RemoteInspectorXPCConnection.mm:153:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/inspector/remote/cocoa/RemoteConnectionToTargetCocoa.mm:164:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/inspector/remote/cocoa/RemoteConnectionToTargetCocoa.mm:199:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/inspector/remote/cocoa/RemoteConnectionToTargetCocoa.mm:217:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 4 in 13 files


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

[Carlos Garcia Campos]

Created attachment 298470 [details]
WIP: Try to fix mac builds

[WebKit Commit Bot]

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


ERROR: Source/JavaScriptCore/inspector/remote/cocoa/RemoteInspectorXPCConnection.mm:153:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/inspector/remote/cocoa/RemoteConnectionToTargetCocoa.mm:164:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/inspector/remote/cocoa/RemoteConnectionToTargetCocoa.mm:199:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/inspector/remote/cocoa/RemoteConnectionToTargetCocoa.mm:217:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 4 in 13 files


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

[Carlos Garcia Campos]

Created attachment 298472 [details]
WIP: Try to fix mac builds

[WebKit Commit Bot]

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


ERROR: Source/JavaScriptCore/inspector/remote/cocoa/RemoteInspectorXPCConnection.mm:153:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/inspector/remote/cocoa/RemoteConnectionToTargetCocoa.mm:164:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/inspector/remote/cocoa/RemoteConnectionToTargetCocoa.mm:199:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/inspector/remote/cocoa/RemoteConnectionToTargetCocoa.mm:217:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 4 in 13 files


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

[Carlos Garcia Campos]

Created attachment 298474 [details]
WIP: Trying to fix mac builds

[WebKit Commit Bot]

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


ERROR: Source/JavaScriptCore/inspector/remote/cocoa/RemoteInspectorXPCConnection.mm:153:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/inspector/remote/cocoa/RemoteConnectionToTargetCocoa.mm:164:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/inspector/remote/cocoa/RemoteConnectionToTargetCocoa.mm:199:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/inspector/remote/cocoa/RemoteConnectionToTargetCocoa.mm:217:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 4 in 13 files


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

[Carlos Garcia Campos]

Created attachment 298557 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/JavaScriptCore/inspector/remote/cocoa/RemoteInspectorXPCConnection.mm:153:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/inspector/remote/cocoa/RemoteConnectionToTargetCocoa.mm:164:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/inspector/remote/cocoa/RemoteConnectionToTargetCocoa.mm:199:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/inspector/remote/cocoa/RemoteConnectionToTargetCocoa.mm:217:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 4 in 15 files


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

[Carlos Garcia Campos]

Ping reviewers. This is just a refactoring.

[Michael Catanzaro]

Comment on attachment 298557 [details]
Patch

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

Looks straightforward, and inspector devs have had plenty of time to complain by now if something's undesired.

> Source/JavaScriptCore/ChangeLog:10
> +        cross-platform parts of RemoteInspector have been moced to a new RemoteInspector.cpp file. Also moved to cocoa

moved

[Blaze Burg]

I'm going to update this patch and land it.

[Blaze Burg]

Committed r212169: <http://trac.webkit.org/changeset/212169>

[Carlos Garcia Campos]

Thanks Brian!

[Csaba Osztrogonác]

(In reply to comment #18)
> Committed r212169: <http://trac.webkit.org/changeset/212169>

Mac CMake buildfix landed in https://trac.webkit.org/changeset/212363

[Alex Christensen]

I'm seeing suspicious crashes on my phone when starting the Web Inspector on iOS:

Thread 0 name:  Dispatch queue: com.apple.main-thread
Thread 0 Crashed:
0   JavaScriptCore                	0x00000001016b86a8 Inspector::InspectorValue::parseJSON(WTF::String const&, WTF::RefPtr<Inspector::InspectorValue>&) + 40
1   JavaScriptCore                	0x000000010166c414 Inspector::BackendDispatcher::dispatch(WTF::String const&) + 104
2   JavaScriptCore                	0x000000010166c414 Inspector::BackendDispatcher::dispatch(WTF::String const&) + 104
3   JavaScriptCore                	0x0000000101956278 ___ZN9Inspector24RemoteConnectionToTarget19sendMessageToTargetERKN3WTF6StringE_block_invoke + 128
4   JavaScriptCore                	0x0000000101956634 Inspector::RemoteTargetHandleRunSourceGlobal(void*) + 220
5   CoreFoundation                	0x000000018fe4e3c8 __CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION__ + 24
6   CoreFoundation                	0x000000018fe4dd38 __CFRunLoopDoSources0 + 540
7   CoreFoundation                	0x000000018fe4b944 __CFRunLoopRun + 744
8   CoreFoundation                	0x000000018fd7b9bc CFRunLoopRunSpecific + 424
9   Foundation                    	0x0000000190897884 -[NSRunLoop(NSRunLoop) runMode:beforeDate:] + 304
10  Foundation                    	0x00000001908ec1d8 -[NSRunLoop(NSRunLoop) run] + 88
11  libxpc.dylib                  	0x000000018ef9fc54 _xpc_objc_main + 664
12  libxpc.dylib                  	0x000000018efa1984 xpc_main + 200
13  com.apple.WebKit.WebContent   	0x00000001000e75b8 main + 376
14  libdyld.dylib                 	0x000000018ed8959c start + 4

[Alex Christensen]

I verified that the WebInspector crashed immediately when starting to inspect a page 100% of the time and I reverted this in http://trac.webkit.org/r212394 and the WebInspector works fine on iOS.
Brian, please check that the WebInspector works on iOS before re-landing this.

[Blaze Burg]

(In reply to comment #22)
> I verified that the WebInspector crashed immediately when starting to
> inspect a page 100% of the time and I reverted this in
> http://trac.webkit.org/r212394 and the WebInspector works fine on iOS.
> Brian, please check that the WebInspector works on iOS before re-landing
> this.

I would have appreciated a ping at the least. It is probably something trivial, and now I'll need to waste some build time on trunk to fix someone else's code.

[Carlos Garcia Campos]

I'll re-check the patch, it shouldn't change anything actually, it's just a refactoring. Let me know if I can help somehow, I can't test on iOS, though.

[Alex Christensen]

Carlos, I'm 100% sure if you re-land the patch as-is it will make the iOS web inspector crash on launch 100% of the time.

[Carlos Garcia Campos]

(In reply to comment #25)
> Carlos, I'm 100% sure if you re-land the patch as-is it will make the iOS
> web inspector crash on launch 100% of the time.

Sure, that's why we need to investigate :-) I must have messed anything in the refactoring.

[Blaze Burg]

(In reply to comment #26)
> (In reply to comment #25)
> > Carlos, I'm 100% sure if you re-land the patch as-is it will make the iOS
> > web inspector crash on launch 100% of the time.
> 
> Sure, that's why we need to investigate :-) I must have messed anything in
> the refactoring.

There was at least one change to RemoteInspector committed to trunk after this patch was last rebased. I had to manually merge in one of them, and must have missed another one. Please compare the pre- and post- of my commit, not the one up on bugzilla.

[Alex Christensen]

Comment on attachment 298557 [details]
Patch

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

> Source/JavaScriptCore/inspector/remote/cocoa/RemoteConnectionToTargetCocoa.mmSource/JavaScriptCore/inspector/remote/RemoteConnectionToTarget.mm:213
> -void RemoteConnectionToTarget::sendMessageToTarget(NSString *message)
> +void RemoteConnectionToTarget::sendMessageToTarget(const String& message)

This change broke it.  I'm going to re-commit it with an NSString.

[Alex Christensen]

Created attachment 301717 [details]
Patch

[Alex Christensen]

Re-committed latest patch with fixed API in http://trac.webkit.org/r212424

[Carlos Garcia Campos]

(In reply to comment #28)
> Comment on attachment 298557 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=298557&action=review
> 
> > Source/JavaScriptCore/inspector/remote/cocoa/RemoteConnectionToTargetCocoa.mmSource/JavaScriptCore/inspector/remote/RemoteConnectionToTarget.mm:213
> > -void RemoteConnectionToTarget::sendMessageToTarget(NSString *message)
> > +void RemoteConnectionToTarget::sendMessageToTarget(const String& message)
> 
> This change broke it.  I'm going to re-commit it with an NSString.

Thanks Alex! Then we need ifdefs, no?. I assumed there was an implicit conversion, because it built.

[Alex Christensen]

I put ifdefs.  We could maybe do something like PlatformString, but that definitely needs to stay an NSString for some reason.

[Joseph Pecoraro]

Comment on attachment 298557 [details]
Patch

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

>>> Source/JavaScriptCore/inspector/remote/cocoa/RemoteConnectionToTargetCocoa.mmSource/JavaScriptCore/inspector/remote/RemoteConnectionToTarget.mm:213
>>> +void RemoteConnectionToTarget::sendMessageToTarget(const String& message)
>> 
>> This change broke it.  I'm going to re-commit it with an NSString.
> 
> Thanks Alex! Then we need ifdefs, no?. I assumed there was an implicit conversion, because it built.

Yes, this would cause issues with WTFString because the String is being transported across multiple threads here.

Here it is on RemoteInspector's XPC Connection Queue and dispatchAsyncOnTarget below will use the string on the Target's Debugger Queue (typically the Main Queue / Web Thread for WebViews).

Using WTFString I think it would destroy the data by the time the other thread uses it. In the NSString case it gets retained for use in the block and released afterwards.

The typical pattern for using WTFString across threads would be an isolatedCopy and capturing in a lambda. It seems that would be rather wasteful here though (the data is not going to change).

[Blaze Burg]

<rdar://problem/30610701>