Bug 155745 - REGRESSION(r198792): [GTK] Inspector crashes in Inspector::Protocol::getEnumConstantValue since r198792
Summary: REGRESSION(r198792): [GTK] Inspector crashes in Inspector::Protocol::getEnumC...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P1 Normal
Assignee: Nobody
URL:
Keywords: Gtk, InRadar, Regression
: 156053 (view as bug list)
Depends on: 155497
Blocks:
  Show dependency treegraph
 
Reported: 2016-03-22 05:18 PDT by Alejandro G. Castro
Modified: 2016-04-04 08:16 PDT (History)
15 users (show)

See Also:


Attachments
Patch (53.52 KB, patch)
2016-03-29 08:42 PDT, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
Updated patch (57.91 KB, patch)
2016-03-30 02:11 PDT, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
Try to fix mac builds (59.60 KB, patch)
2016-03-30 02:34 PDT, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
Rebased patch to fix the build (61.00 KB, patch)
2016-03-31 00:10 PDT, Carlos Garcia Campos
bburg: review+
bburg: commit-queue-
Details | Formatted Diff | Diff
Patch for landing (65.91 KB, patch)
2016-04-03 04:18 PDT, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alejandro G. Castro 2016-03-22 05:18:17 PDT
Just open the inspector after applying patch in bug 155497 and try to execute any command in the console, this is the backtrace I get:

Program received signal SIGSEGV, Segmentation fault.
strlen () at ../sysdeps/x86_64/strlen.S:106
106		movdqu	(%rax), %xmm12
(gdb) bt
#0  strlen () at ../sysdeps/x86_64/strlen.S:106
#1  0x00007fe403bce3c9 in WTF::StringImpl::create(unsigned char const*) () from /home/alex/checkout/WebKit/WebKitBuild/Release/lib/libjavascriptcoregtk-4.0.so.18
#2  0x00007fe403bdc12c in WTF::String::String(unsigned char const*) () from /home/alex/checkout/WebKit/WebKitBuild/Release/lib/libjavascriptcoregtk-4.0.so.18
#3  0x00007fe4052074c7 in Inspector::Protocol::getEnumConstantValue(int) () from /home/alex/checkout/WebKit/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37
#4  0x00007fe403b90f46 in Inspector::RuntimeBackendDispatcher::parse(long, WTF::RefPtr<Inspector::InspectorObject>&&) () from /home/alex/checkout/WebKit/WebKitBuild/Release/lib/libjavascriptcoregtk-4.0.so.18
#5  0x00007fe403b83c90 in Inspector::RuntimeBackendDispatcher::dispatch(long, WTF::String const&, WTF::Ref<Inspector::InspectorObject>&&) () from /home/alex/checkout/WebKit/WebKitBuild/Release/lib/libjavascriptcoregtk-4.0.so.18
#6  0x00007fe40375da18 in Inspector::BackendDispatcher::dispatch(WTF::String const&) () from /home/alex/checkout/WebKit/WebKitBuild/Release/lib/libjavascriptcoregtk-4.0.so.18
#7  0x00007fe40523ea02 in WebKit::WebInspector::didReceiveMessage(IPC::Connection&, IPC::MessageDecoder&) () from /home/alex/checkout/WebKit/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37
#8  0x00007fe404f6ae46 in IPC::Connection::dispatchMessage(std::unique_ptr<IPC::MessageDecoder, std::default_delete<IPC::MessageDecoder> >) () from /home/alex/checkout/WebKit/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37
#9  0x00007fe404f6b873 in IPC::Connection::dispatchOneMessage() () from /home/alex/checkout/WebKit/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37
#10 0x00007fe403bbb25f in WTF::RunLoop::performWork() () from /home/alex/checkout/WebKit/WebKitBuild/Release/lib/libjavascriptcoregtk-4.0.so.18
#11 0x00007fe403bea289 in WTF::RunLoop::RunLoop()::{lambda(void*)#1}::_FUN(void*) () from /home/alex/checkout/WebKit/WebKitBuild/Release/lib/libjavascriptcoregtk-4.0.so.18
#12 0x00007fe3fe4d181a in g_main_dispatch (context=0x1bfcf20) at /home/alex/checkout/WebKit/WebKitBuild/DependenciesGTK/Source/glib-2.44.1/glib/gmain.c:3122
#13 g_main_context_dispatch (context=context@entry=0x1bfcf20) at /home/alex/checkout/WebKit/WebKitBuild/DependenciesGTK/Source/glib-2.44.1/glib/gmain.c:3737
#14 0x00007fe3fe4d1b98 in g_main_context_iterate (context=0x1bfcf20, block=block@entry=1, dispatch=dispatch@entry=1, self=<optimized out>) at /home/alex/checkout/WebKit/WebKitBuild/DependenciesGTK/Source/glib-2.44.1/glib/gmain.c:3808
#15 0x00007fe3fe4d1eb2 in g_main_loop_run (loop=0x22d0ce0) at /home/alex/checkout/WebKit/WebKitBuild/DependenciesGTK/Source/glib-2.44.1/glib/gmain.c:4002
#16 0x00007fe403beaab0 in WTF::RunLoop::run() () from /home/alex/checkout/WebKit/WebKitBuild/Release/lib/libjavascriptcoregtk-4.0.so.18
#17 0x00007fe405200e72 in int WebKit::ChildProcessMain<WebKit::WebProcess, WebKit::WebProcessMain>(int, char**) () from /home/alex/checkout/WebKit/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37
#18 0x00007fe3f9703580 in __libc_start_main (main=0x400b40 <main>, argc=2, argv=0x7ffd373b4f48, init=<optimized out>, fini=<optimized out>, rtld_fini=<optimized out>, stack_end=0x7ffd373b4f38) at libc-start.c:289
Comment 1 Radar WebKit Bug Importer 2016-03-22 05:18:39 PDT
<rdar://problem/25289456>
Comment 2 Carlos Garcia Campos 2016-03-29 08:42:14 PDT
Created attachment 275093 [details]
Patch
Comment 3 WebKit Commit Bot 2016-03-29 08:44:48 PDT
This patch modifies the inspector protocol generator. Please ensure that you have rebaselined any generator test results (i.e., by running `Tools/Scripts/run-inspector-generator-tests --reset-results`)
Comment 4 WebKit Commit Bot 2016-03-29 08:44:57 PDT
Attachment 275093 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_cpp_protocol_types_header.py:76:  [CppProtocolTypesHeaderGenerator.generate_output] Instance of 'CppProtocolTypesHeaderGenerator' has no 'enums_namespace' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_cpp_protocol_types_header.py:83:  [CppProtocolTypesHeaderGenerator.generate_output] Instance of 'CppProtocolTypesHeaderGenerator' has no 'enums_namespace' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_cpp_protocol_types_header.py:87:  [CppProtocolTypesHeaderGenerator.generate_output] Instance of 'CppProtocolTypesHeaderGenerator' has no 'enums_namespace' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_cpp_protocol_types_header.py:89:  [CppProtocolTypesHeaderGenerator.generate_output] Instance of 'CppProtocolTypesHeaderGenerator' has no 'enums_namespace' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_cpp_protocol_types_header.py:288:  [CppProtocolTypesHeaderGenerator._generate_builder_setter_for_member] Instance of 'CppProtocolTypesHeaderGenerator' has no 'enums_namespace' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_cpp_protocol_types_header.py:311:  [CppProtocolTypesHeaderGenerator._generate_unchecked_setter_for_member] Instance of 'CppProtocolTypesHeaderGenerator' has no 'enums_namespace' member  [pylint/E1101] [5]
Total errors found: 6 in 26 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 5 Brian Burg 2016-03-29 08:57:40 PDT
Comment on attachment 275093 [details]
Patch

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

Great work! Just a few naming suggestions, and let's wait for EWS.

It's weird that this crash happens on GTK but not Mac since we generate the same protocol groups.
In the longer term there won't be a need for client code (inspector Agents etc) to use this helper directly because the protocol bindings will do the type checking and conversions itself.

> Source/JavaScriptCore/ChangeLog:19
> +        getEnumConstantValue() generated and used foe every framework, so

Nit: 'for'

> Source/JavaScriptCore/ChangeLog:25
> +        (CppGenerator.enums_namespace): Return the namespace name that

Suggestion: If you are going to line wrap with this few columns, it's a lot easier to parse the changelog if you add newlines after each text block.

> Source/JavaScriptCore/inspector/agents/InspectorDebuggerAgent.cpp:219
> +    if (typeString == Inspector::Protocol::InspectorEnums::getEnumConstantValue(Inspector::Protocol::Debugger::BreakpointAction::Type::Log)) {

Please change the namespace to XXXHelpers or XXXConversions. There are other helpers we may want to put in a disambiguated namespace.

> Source/JavaScriptCore/inspector/scripts/codegen/cpp_generator.py:53
> +    def enums_namespace(self):

As above, please rename this function to helper_namespace or similar.

> Source/JavaScriptCore/inspector/scripts/codegen/generate_cpp_protocol_types_header.py:76
> +        sections.append('namespace %s {' % self.enums_namespace())

I would recommend extracting this entire snippet into a private method, the top-level generation method is getting long.

> Source/JavaScriptCore/inspector/scripts/codegen/generate_cpp_protocol_types_header.py:89
> +        sections.append('} // namespace %s' % self.enums_namespace())

Please inline the namespace wrapping into self._generate_declarations_for_enum_conversion_methods() and do not emit it if there are no lines emitted into the namespace.

> Source/JavaScriptCore/inspector/scripts/codegen/generate_cpp_protocol_types_header.py:298
> +            lines.append('            m_result->%(keyedSet)s(ASCIILiteral("%(name)s"), Inspector::Protocol::%(enumsNamespace)s::getEnumConstantValue(value));' % setter_args)

I am surprised that removing the cast doesn't cause a compiler error.

> Source/JavaScriptCore/inspector/scripts/codegen/generate_cpp_protocol_types_implementation.py:70
> +        sections.append('} // namespace %s' % self.enums_namespace())

See comment above.

> Source/JavaScriptCore/inspector/scripts/tests/expected/commands-with-async-attribute.json-result:814
> +

This extra newline isn't needed.
Comment 6 Alejandro G. Castro 2016-03-29 09:12:12 PDT
Comment on attachment 275093 [details]
Patch

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

Just tested it, it works for me, thanks. Apparently the linker was not sorting out the situation with 2 definitions of a symbol very well, maybe in case of Apple their linker was smarter or just luckier.

> Source/JavaScriptCore/ChangeLog:15
> +        always end up using the one in WebKit2. Since the

Did you mean JavaScriptCore here?

>> Source/JavaScriptCore/inspector/scripts/codegen/generate_cpp_protocol_types_header.py:298
>> +            lines.append('            m_result->%(keyedSet)s(ASCIILiteral("%(name)s"), Inspector::Protocol::%(enumsNamespace)s::getEnumConstantValue(value));' % setter_args)
> 
> I am surprised that removing the cast doesn't cause a compiler error.

The function with T type used that does the cast to int is defined up in the file.
Comment 7 Carlos Garcia Campos 2016-03-29 23:56:27 PDT
(In reply to comment #5)
> Comment on attachment 275093 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=275093&action=review
> 
> Great work! Just a few naming suggestions, and let's wait for EWS.

Thanks for the review.

> It's weird that this crash happens on GTK but not Mac since we generate the
> same protocol groups.

I was also surprised, my guess is that for some reason in Mac you always end up using the JSC table that is bigger.

> In the longer term there won't be a need for client code (inspector Agents
> etc) to use this helper directly because the protocol bindings will do the
> type checking and conversions itself.
> 
> > Source/JavaScriptCore/ChangeLog:19
> > +        getEnumConstantValue() generated and used foe every framework, so
> 
> Nit: 'for'

Oops.

> > Source/JavaScriptCore/ChangeLog:25
> > +        (CppGenerator.enums_namespace): Return the namespace name that
> 
> Suggestion: If you are going to line wrap with this few columns, it's a lot
> easier to parse the changelog if you add newlines after each text block.

It's emacs default :-) I'll try to make it easier to read either adding new lines or configuring emacs to use longer lines.

> > Source/JavaScriptCore/inspector/agents/InspectorDebuggerAgent.cpp:219
> > +    if (typeString == Inspector::Protocol::InspectorEnums::getEnumConstantValue(Inspector::Protocol::Debugger::BreakpointAction::Type::Log)) {
> 
> Please change the namespace to XXXHelpers or XXXConversions. There are other
> helpers we may want to put in a disambiguated namespace.

Helpers is more generic, just in case we end up adding something that is not a conversor.

> > Source/JavaScriptCore/inspector/scripts/codegen/cpp_generator.py:53
> > +    def enums_namespace(self):
> 
> As above, please rename this function to helper_namespace or similar.

Sure.

> > Source/JavaScriptCore/inspector/scripts/codegen/generate_cpp_protocol_types_header.py:76
> > +        sections.append('namespace %s {' % self.enums_namespace())
> 
> I would recommend extracting this entire snippet into a private method, the
> top-level generation method is getting long.

Ok.

> > Source/JavaScriptCore/inspector/scripts/codegen/generate_cpp_protocol_types_header.py:89
> > +        sections.append('} // namespace %s' % self.enums_namespace())
> 
> Please inline the namespace wrapping into
> self._generate_declarations_for_enum_conversion_methods() and do not emit it
> if there are no lines emitted into the namespace.

Ok.

> > Source/JavaScriptCore/inspector/scripts/codegen/generate_cpp_protocol_types_header.py:298
> > +            lines.append('            m_result->%(keyedSet)s(ASCIILiteral("%(name)s"), Inspector::Protocol::%(enumsNamespace)s::getEnumConstantValue(value));' % setter_args)
> 
> I am surprised that removing the cast doesn't cause a compiler error.

We have a template function that makes the cast already.

> > Source/JavaScriptCore/inspector/scripts/codegen/generate_cpp_protocol_types_implementation.py:70
> > +        sections.append('} // namespace %s' % self.enums_namespace())
> 
> See comment above.
> 
> > Source/JavaScriptCore/inspector/scripts/tests/expected/commands-with-async-attribute.json-result:814
> > +
> 
> This extra newline isn't needed.

That was --reset-results I haven't changed tests results manually.
Comment 8 Carlos Garcia Campos 2016-03-29 23:57:14 PDT
(In reply to comment #6)
> Comment on attachment 275093 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=275093&action=review
> 
> Just tested it, it works for me, thanks. Apparently the linker was not
> sorting out the situation with 2 definitions of a symbol very well, maybe in
> case of Apple their linker was smarter or just luckier.
> 
> > Source/JavaScriptCore/ChangeLog:15
> > +        always end up using the one in WebKit2. Since the
> 
> Did you mean JavaScriptCore here?

No, I mean the WebKit2 one which is the small one.
Comment 9 Carlos Garcia Campos 2016-03-30 02:10:27 PDT
(In reply to comment #7)
> > > Source/JavaScriptCore/inspector/scripts/tests/expected/commands-with-async-attribute.json-result:814
> > > +
> > 
> > This extra newline isn't needed.
> 
> That was --reset-results I haven't changed tests results manually.

I see what you mean now. While it's quite obvious when looking at the generated code, it's not that easy from the generator code, since we don't know if we will have something else after that or another namespace closed.
Comment 10 Carlos Garcia Campos 2016-03-30 02:11:09 PDT
Created attachment 275188 [details]
Updated patch

I think I've addressed all review comments.
Comment 11 WebKit Commit Bot 2016-03-30 02:12:14 PDT
Attachment 275188 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_cpp_protocol_types_header.py:153:  [CppProtocolTypesHeaderGenerator._generate_enum_constant_value_conversion_methods] Instance of 'CppProtocolTypesHeaderGenerator' has no 'assigned_enum_values' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_cpp_protocol_types_header.py:158:  [CppProtocolTypesHeaderGenerator._generate_enum_constant_value_conversion_methods] Instance of 'CppProtocolTypesHeaderGenerator' has no 'model' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_cpp_protocol_types_header.py:163:  [CppProtocolTypesHeaderGenerator._generate_enum_constant_value_conversion_methods] Instance of 'CppProtocolTypesHeaderGenerator' has no 'helpers_namespace' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_cpp_protocol_types_header.py:172:  [CppProtocolTypesHeaderGenerator._generate_enum_constant_value_conversion_methods] Instance of 'CppProtocolTypesHeaderGenerator' has no 'helpers_namespace' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_cpp_protocol_types_header.py:295:  [CppProtocolTypesHeaderGenerator._generate_builder_setter_for_member] Instance of 'CppProtocolTypesHeaderGenerator' has no 'helpers_namespace' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_cpp_protocol_types_header.py:318:  [CppProtocolTypesHeaderGenerator._generate_unchecked_setter_for_member] Instance of 'CppProtocolTypesHeaderGenerator' has no 'helpers_namespace' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_cpp_protocol_types_header.py:373:  [CppProtocolTypesHeaderGenerator._generate_declarations_for_enum_conversion_methods.return_type_with_export_macro] Instance of 'CppProtocolTypesHeaderGenerator' has no 'model' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_cpp_protocol_types_header.py:413:  [CppProtocolTypesHeaderGenerator._generate_declarations_for_enum_conversion_methods] Instance of 'CppProtocolTypesHeaderGenerator' has no 'helpers_namespace' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_cpp_protocol_types_header.py:418:  [CppProtocolTypesHeaderGenerator._generate_declarations_for_enum_conversion_methods] Instance of 'CppProtocolTypesHeaderGenerator' has no 'helpers_namespace' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_cpp_protocol_types_implementation.py:78:  [CppProtocolTypesImplementationGenerator._generate_enum_mapping] Instance of 'CppProtocolTypesImplementationGenerator' has no 'assigned_enum_values' member  [pylint/E1101] [5]
Total errors found: 10 in 20 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 12 Carlos Garcia Campos 2016-03-30 02:34:40 PDT
Created attachment 275189 [details]
Try to fix mac builds
Comment 13 WebKit Commit Bot 2016-03-30 02:37:09 PDT
Attachment 275189 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_cpp_protocol_types_header.py:153:  [CppProtocolTypesHeaderGenerator._generate_enum_constant_value_conversion_methods] Instance of 'CppProtocolTypesHeaderGenerator' has no 'assigned_enum_values' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_cpp_protocol_types_header.py:158:  [CppProtocolTypesHeaderGenerator._generate_enum_constant_value_conversion_methods] Instance of 'CppProtocolTypesHeaderGenerator' has no 'model' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_cpp_protocol_types_header.py:163:  [CppProtocolTypesHeaderGenerator._generate_enum_constant_value_conversion_methods] Instance of 'CppProtocolTypesHeaderGenerator' has no 'helpers_namespace' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_cpp_protocol_types_header.py:172:  [CppProtocolTypesHeaderGenerator._generate_enum_constant_value_conversion_methods] Instance of 'CppProtocolTypesHeaderGenerator' has no 'helpers_namespace' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_cpp_protocol_types_header.py:295:  [CppProtocolTypesHeaderGenerator._generate_builder_setter_for_member] Instance of 'CppProtocolTypesHeaderGenerator' has no 'helpers_namespace' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_cpp_protocol_types_header.py:318:  [CppProtocolTypesHeaderGenerator._generate_unchecked_setter_for_member] Instance of 'CppProtocolTypesHeaderGenerator' has no 'helpers_namespace' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_cpp_protocol_types_header.py:373:  [CppProtocolTypesHeaderGenerator._generate_declarations_for_enum_conversion_methods.return_type_with_export_macro] Instance of 'CppProtocolTypesHeaderGenerator' has no 'model' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_cpp_protocol_types_header.py:413:  [CppProtocolTypesHeaderGenerator._generate_declarations_for_enum_conversion_methods] Instance of 'CppProtocolTypesHeaderGenerator' has no 'helpers_namespace' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_cpp_protocol_types_header.py:418:  [CppProtocolTypesHeaderGenerator._generate_declarations_for_enum_conversion_methods] Instance of 'CppProtocolTypesHeaderGenerator' has no 'helpers_namespace' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_cpp_protocol_types_implementation.py:78:  [CppProtocolTypesImplementationGenerator._generate_enum_mapping] Instance of 'CppProtocolTypesImplementationGenerator' has no 'assigned_enum_values' member  [pylint/E1101] [5]
Total errors found: 10 in 20 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 14 Carlos Garcia Campos 2016-03-30 23:50:30 PDT
*** Bug 156053 has been marked as a duplicate of this bug. ***
Comment 15 Carlos Garcia Campos 2016-03-31 00:10:09 PDT
Created attachment 275268 [details]
Rebased patch to fix the build

Rebase needed after r198872
Comment 16 WebKit Commit Bot 2016-03-31 00:12:49 PDT
Attachment 275268 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_cpp_protocol_types_header.py:153:  [CppProtocolTypesHeaderGenerator._generate_enum_constant_value_conversion_methods] Instance of 'CppProtocolTypesHeaderGenerator' has no 'assigned_enum_values' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_cpp_protocol_types_header.py:158:  [CppProtocolTypesHeaderGenerator._generate_enum_constant_value_conversion_methods] Instance of 'CppProtocolTypesHeaderGenerator' has no 'model' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_cpp_protocol_types_header.py:163:  [CppProtocolTypesHeaderGenerator._generate_enum_constant_value_conversion_methods] Instance of 'CppProtocolTypesHeaderGenerator' has no 'helpers_namespace' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_cpp_protocol_types_header.py:172:  [CppProtocolTypesHeaderGenerator._generate_enum_constant_value_conversion_methods] Instance of 'CppProtocolTypesHeaderGenerator' has no 'helpers_namespace' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_cpp_protocol_types_header.py:295:  [CppProtocolTypesHeaderGenerator._generate_builder_setter_for_member] Instance of 'CppProtocolTypesHeaderGenerator' has no 'helpers_namespace' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_cpp_protocol_types_header.py:318:  [CppProtocolTypesHeaderGenerator._generate_unchecked_setter_for_member] Instance of 'CppProtocolTypesHeaderGenerator' has no 'helpers_namespace' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_cpp_protocol_types_header.py:373:  [CppProtocolTypesHeaderGenerator._generate_declarations_for_enum_conversion_methods.return_type_with_export_macro] Instance of 'CppProtocolTypesHeaderGenerator' has no 'model' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_cpp_protocol_types_header.py:413:  [CppProtocolTypesHeaderGenerator._generate_declarations_for_enum_conversion_methods] Instance of 'CppProtocolTypesHeaderGenerator' has no 'helpers_namespace' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_cpp_protocol_types_header.py:418:  [CppProtocolTypesHeaderGenerator._generate_declarations_for_enum_conversion_methods] Instance of 'CppProtocolTypesHeaderGenerator' has no 'helpers_namespace' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_cpp_protocol_types_implementation.py:78:  [CppProtocolTypesImplementationGenerator._generate_enum_mapping] Instance of 'CppProtocolTypesImplementationGenerator' has no 'assigned_enum_values' member  [pylint/E1101] [5]
Total errors found: 10 in 20 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 17 Carlos Alberto Lopez Perez 2016-04-01 06:01:24 PDT
Reviewers? ping?

This is making the inspector crash like mad in the GTK+ port.
Comment 18 Carlos Alberto Lopez Perez 2016-04-01 06:12:40 PDT
I'm trying to build this on top of r198945 and I'm getting build errors: http://sprunge.us/MdXG


I guess some revision between the upload and r198945 broke the build for this patch.
Comment 19 Brian Burg 2016-04-01 07:36:43 PDT
Comment on attachment 275268 [details]
Rebased patch to fix the build

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

r=me

Just some style issues remain. Please post an updated patch so we can set cq+ on it.

> Source/JavaScriptCore/inspector/scripts/codegen/generate_cpp_protocol_types_header.py:173
> +        return ['\n\n'.join(lines)]

The pattern used in these generators is that lines are joined by one newline, and sections are joined by two newlines. So you could just return the array here and the caller will join it the same way.

> Source/JavaScriptCore/inspector/scripts/codegen/generate_cpp_protocol_types_header.py:412
> +        sections.insert(0, '\n'.join([

Why do we need to use insert here? The generator's sections should be in the same order as the generated sections.

> Source/JavaScriptCore/inspector/scripts/codegen/generate_cpp_protocol_types_implementation.py:154
> +        sections.insert(0, 'namespace %s {' % self.helpers_namespace())

Instead of using insert and causing the generator and generated file to have different line order, please use a guard at the top like 'if not self.assigned_enum_values()' and return [] before generating sections.

> Source/JavaScriptCore/inspector/scripts/codegen/generate_cpp_protocol_types_implementation.py:156
> +        return ['\n\n'.join(sections)]

See above comment, should not be necessary to join sections here if the caller can extend with this method's result.
Comment 20 Brian Burg 2016-04-01 07:38:42 PDT
(In reply to comment #19)
> Comment on attachment 275268 [details]
> Rebased patch to fix the build
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=275268&action=review
> 
> r=me
> 
> Just some style issues remain. Please post an updated patch so we can set
> cq+ on it.
> 

The reason I ask is that we have been landing a lot of code in the WK2 parts lately, so this is unlikely to build anymore as-is.
Comment 21 Carlos Garcia Campos 2016-04-03 04:18:07 PDT
Created attachment 275494 [details]
Patch for landing
Comment 22 WebKit Commit Bot 2016-04-03 04:20:13 PDT
Attachment 275494 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_cpp_protocol_types_header.py:416:  at least two spaces before inline comment  [pep8/E261] [5]
ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_cpp_protocol_types_header.py:153:  [CppProtocolTypesHeaderGenerator._generate_enum_constant_value_conversion_methods] Instance of 'CppProtocolTypesHeaderGenerator' has no 'assigned_enum_values' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_cpp_protocol_types_header.py:158:  [CppProtocolTypesHeaderGenerator._generate_enum_constant_value_conversion_methods] Instance of 'CppProtocolTypesHeaderGenerator' has no 'model' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_cpp_protocol_types_header.py:163:  [CppProtocolTypesHeaderGenerator._generate_enum_constant_value_conversion_methods] Instance of 'CppProtocolTypesHeaderGenerator' has no 'helpers_namespace' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_cpp_protocol_types_header.py:172:  [CppProtocolTypesHeaderGenerator._generate_enum_constant_value_conversion_methods] Instance of 'CppProtocolTypesHeaderGenerator' has no 'helpers_namespace' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_cpp_protocol_types_header.py:295:  [CppProtocolTypesHeaderGenerator._generate_builder_setter_for_member] Instance of 'CppProtocolTypesHeaderGenerator' has no 'helpers_namespace' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_cpp_protocol_types_header.py:318:  [CppProtocolTypesHeaderGenerator._generate_unchecked_setter_for_member] Instance of 'CppProtocolTypesHeaderGenerator' has no 'helpers_namespace' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_cpp_protocol_types_header.py:370:  [CppProtocolTypesHeaderGenerator._generate_declarations_for_enum_conversion_methods] Instance of 'CppProtocolTypesHeaderGenerator' has no 'helpers_namespace' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_cpp_protocol_types_header.py:379:  [CppProtocolTypesHeaderGenerator._generate_declarations_for_enum_conversion_methods.return_type_with_export_macro] Instance of 'CppProtocolTypesHeaderGenerator' has no 'model' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_cpp_protocol_types_header.py:418:  [CppProtocolTypesHeaderGenerator._generate_declarations_for_enum_conversion_methods] Instance of 'CppProtocolTypesHeaderGenerator' has no 'helpers_namespace' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_cpp_protocol_types_implementation.py:78:  [CppProtocolTypesImplementationGenerator._generate_enum_mapping] Instance of 'CppProtocolTypesImplementationGenerator' has no 'assigned_enum_values' member  [pylint/E1101] [5]
Total errors found: 11 in 20 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 23 WebKit Commit Bot 2016-04-04 07:30:31 PDT
Comment on attachment 275494 [details]
Patch for landing

Clearing flags on attachment: 275494

Committed r199005: <http://trac.webkit.org/changeset/199005>
Comment 24 WebKit Commit Bot 2016-04-04 07:30:38 PDT
All reviewed patches have been landed.  Closing bug.
Comment 25 Csaba Osztrogonác 2016-04-04 07:34:46 PDT
(In reply to comment #23)
> Comment on attachment 275494 [details]
> Patch for landing
> 
> Clearing flags on attachment: 275494
> 
> Committed r199005: <http://trac.webkit.org/changeset/199005>

It broke the WinCairo build: https://build.webkit.org/builders/WinCairo%2064-Bit%20Release/builds/55657
Comment 26 Csaba Osztrogonác 2016-04-04 08:16:59 PDT
(In reply to comment #25)
> (In reply to comment #23)
> > Comment on attachment 275494 [details]
> > Patch for landing
> > 
> > Clearing flags on attachment: 275494
> > 
> > Committed r199005: <http://trac.webkit.org/changeset/199005>
> 
> It broke the WinCairo build:
> https://build.webkit.org/builders/WinCairo%2064-Bit%20Release/builds/55657

It was false positive alarm, clean build fixed the build. It would be great
if the port maintainers could investigate and fix this build system bug.