Bug 56041 – RexExp constructor should only accept flags "gim"

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Gavin Barraclough]

We also should be passing the flags around as a bitfield rather than a string, and should not have redundant, incompatible code for converting the string to a bitfield!

[Gavin Barraclough]

Created attachment 85213 [details]
The patch

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/sput..." exit_code: 1

Source/JavaScriptCore/runtime/RegExp.h:40:  The parameter name "globalData" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/runtime/RegExp.h:40:  The parameter name "flags" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/runtime/RegExp.h:60:  The parameter name "globalData" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/runtime/RegExp.h:60:  The parameter name "flags" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/runtime/RegExpCache.h:44:  The parameter name "flags" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/runtime/RegExpCache.h:45:  The parameter name "flags" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/JavaScriptCore/runtime/RegExpCache.h:45:  The parameter name "iterator" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 7 in 23 files


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

[Darin Adler]

Comment on attachment 85213 [details]
The patch

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

> Source/JavaScriptCore/runtime/RegExp.h:27
> +#include "RegExpKey.h"

Seems a little awkward to have to include RegExpKey here to get this flags enum.

>>> Source/JavaScriptCore/runtime/RegExp.h:40
>>> +        static PassRefPtr<RegExp> create(JSGlobalData* globalData, const UString& pattern, RegExpFlags flags);
>> 
>> The parameter name "globalData" adds no information, so it should be removed.  [readability/parameter_name] [5]
> 
> The parameter name "flags" adds no information, so it should be removed.  [readability/parameter_name] [5]

I agree with the stylebot.

>>> Source/JavaScriptCore/runtime/RegExp.h:60
>>> +        RegExp(JSGlobalData* globalData, const UString& pattern, RegExpFlags flags);
>> 
>> The parameter name "globalData" adds no information, so it should be removed.  [readability/parameter_name] [5]
> 
> The parameter name "flags" adds no information, so it should be removed.  [readability/parameter_name] [5]

I agree with the stylebot.

>> Source/JavaScriptCore/runtime/RegExpCache.h:44
>> +    PassRefPtr<RegExp> lookupOrCreate(const UString& patternString, RegExpFlags flags);
> 
> The parameter name "flags" adds no information, so it should be removed.  [readability/parameter_name] [5]

I agree with the stylebot.

>>> Source/JavaScriptCore/runtime/RegExpCache.h:45
>>> +    PassRefPtr<RegExp> create(const UString& patternString, RegExpFlags flags, RegExpCacheMap::iterator iterator);
>> 
>> The parameter name "flags" adds no information, so it should be removed.  [readability/parameter_name] [5]
> 
> The parameter name "iterator" adds no information, so it should be removed.  [readability/parameter_name] [5]

I agree with the stylebot.

> Source/JavaScriptCore/runtime/RegExpConstructor.cpp:313
> +        flags = regExpFlags(arg1.toString(exec));

Should return here if toString raised an exception rather than continuing with NoFlags.

> Source/JavaScriptCore/runtime/RegExpKey.h:36
> +enum RegExpFlags {

Flag bits are not quite an enum since they can be enum values or'ed together. In some places we’ve used a separate integral typedef for this. I guess this is OK.

> Source/JavaScriptCore/runtime/RegExpPrototype.cpp:98
> +            flags = regExpFlags(arg1.toString(exec));

Should return here if toString raised an exception rather than continuing with NoFlags.

[Build Bot]

Attachment 85213 [details] did not build on win:
Build output: http://queues.webkit.org/results/8118525

[Early Warning System Bot]

Attachment 85213 [details] did not build on qt:
Build output: http://queues.webkit.org/results/8118549

[Gavin Barraclough]

(In reply to comment #3)

> > Source/JavaScriptCore/runtime/RegExp.h:27
> > +#include "RegExpKey.h"
> 
> Seems a little awkward to have to include RegExpKey here to get this flags enum.

Agreed, but I didn't have a better solution.  RegExpKey comes quite high up the food chain (included by JSGlobalData.h), and it seems that RegExpKey really needs to know about this type.  If you have any suggestions I'll be happy to follow up.

Fixed stylebot issues / missing toString exception checks.

> > Source/JavaScriptCore/runtime/RegExpKey.h:36
> > +enum RegExpFlags {
> 
> Flag bits are not quite an enum since they can be enum values or'ed together. In some places we’ve used a separate integral typedef for this. I guess this is OK.

Whilst it is a slight abuse of the language I strongly prefer an enum to get static type checking to catch errors.  Since you've said okay I won't change, give me a stronger signal that you don't like this if you want me to change my approach here. :-)

cheers,
G.

[Gavin Barraclough]

Fixed in r80667

[Peter Kasting]

This commit broke the Qt bots, see e.g. http://build.webkit.org/builders/Qt%20Windows%2032-bit%20Debug/builds/17389/steps/compile-webkit/logs/stdio

[Peter Kasting]

(In reply to comment #8)
> This commit broke the Qt bots, see e.g. http://build.webkit.org/builders/Qt%20Windows%2032-bit%20Debug/builds/17389/steps/compile-webkit/logs/stdio

...and Windows as well.  I notice the EWS bots in both these cases flagged this before landing.  Maybe this shouldn't have landed?

[Gavin Barraclough]

Hi peter,

Apologies for the breakage, I'm fixing this now.

Just wanted to let you know, I did fix the issues identified by the EWS, unfortunately this patch managed to break the same two builds in different ways, too. :-(

cheers,
G.

[Daniel Bates]

(In reply to comment #10)
> Hi peter,
> 
> Apologies for the breakage, I'm fixing this now.

For completeness, it appears that this fix was landed in changeset 80684 <http://trac.webkit.org/changeset/80684>.

[Daniel Bates]

Changeset 80684 <http://trac.webkit.org/changeset/80684> broke the WinCE build.

Build fix committed in changeset 80691 <http://trac.webkit.org/changeset/80691>.

[Philippe Normand]

It seems this patch makes 2 tests on GTK crash sometimes:

fast/regex/parentheses.html -> crashed
fast/regex/pcre-test-1.html -> crashed

#0  0x00002b947b5ef542 in JSC::RegExpNode::emitBytecode (this=0x12d6838, generator=..., dst=0x13c1e2c) at ../../Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:129
129	    ASSERT(flags != InvalidFlags);

Thread 1 (Thread 1535):
#0  0x00002b947b5ef542 in JSC::RegExpNode::emitBytecode (this=0x12d6838, generator=..., dst=0x13c1e2c) at ../../Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:129
#1  0x00002b947b5d8d83 in JSC::BytecodeGenerator::emitNode (this=0x13bfe90, dst=0x13c1e2c, n=0x12d6838) at ../../Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:212
#2  0x00002b947b5f6e17 in JSC::AssignResolveNode::emitBytecode (this=0x12d6860, generator=..., dst=0x0) at ../../Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:1198
#3  0x00002b947b5d8d83 in JSC::BytecodeGenerator::emitNode (this=0x13bfe90, dst=0x0, n=0x12d6860) at ../../Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:212
#4  0x00002b947b5fd539 in JSC::BytecodeGenerator::emitNode (this=0x13bfe90, n=0x12d6860) at ../../Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:217
#5  0x00002b947b5f8161 in JSC::VarStatementNode::emitBytecode (this=0x12d6898, generator=...) at ../../Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:1403
#6  0x00002b947b5d8d83 in JSC::BytecodeGenerator::emitNode (this=0x13bfe90, dst=0x13c01f8, n=0x12d6898) at ../../Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:212
#7  0x00002b947b5fd9d8 in JSC::SourceElements::emitBytecode (this=0xc32e90, generator=..., dst=0x13c01f8) at ../../Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:1350
#8  0x00002b947b5fdaef in JSC::ScopeNode::emitStatementsBytecode (this=0x12af170, generator=..., dst=0x13c01f8) at ../../Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:1968
#9  0x00002b947b5fbb2d in JSC::ProgramNode::emitBytecode (this=0x12af170, generator=...) at ../../Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:1979
#10 0x00002b947b5c9efe in JSC::BytecodeGenerator::generate (this=0x13bfe90) at ../../Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:144
#11 0x00002b947b6b0dbb in JSC::ProgramExecutable::compileInternal (this=0x2b94d08341d8, exec=0x2b948c002538, scopeChainNode=0x2b94d0824188) at ../../Source/JavaScriptCore/runtime/Executable.cpp:173
#12 0x00002b947b610f04 in JSC::ProgramExecutable::compile (this=0x2b94d08341d8, exec=0x2b948c002538, scopeChainNode=0x2b94d0824188) at ../../Source/JavaScriptCore/runtime/Executable.h:265
#13 0x00002b947b6a6658 in JSC::evaluate (exec=0x2b948c002538, scopeChain=0x2b94d0824188, source=..., thisValue=...) at ../../Source/JavaScriptCore/runtime/Completion.cpp:61
#14 0x00002b947a8d0d0f in WebCore::JSMainThreadExecState::evaluate (exec=0x2b948c002538, chain=0x2b94d0824188, source=..., thisValue=...) at ../../Source/WebCore/bindings/js/JSMainThreadExecState.h:54
#15 0x00002b947a8f78a2 in WebCore::ScriptController::evaluateInWorld (this=0xc8e0e8, sourceCode=..., world=0x12ac390) at ../../Source/WebCore/bindings/js/ScriptController.cpp:142
#16 0x00002b947a8f7a62 in WebCore::ScriptController::evaluate (this=0xc8e0e8, sourceCode=...) at ../../Source/WebCore/bindings/js/ScriptController.cpp:165
#17 0x00002b947ab07e1d in WebCore::ScriptElement::executeScript (this=0x2b948c009e00, sourceCode=...) at ../../Source/WebCore/dom/ScriptElement.cpp:256
#18 0x00002b947ac8f866 in WebCore::HTMLScriptRunner::executePendingScriptAndDispatchEvent (this=0xc328f0, pendingScript=...) at ../../Source/WebCore/html/parser/HTMLScriptRunner.cpp:144
#19 0x00002b947ac8f68a in WebCore::HTMLScriptRunner::executeParsingBlockingScript (this=0xc328f0) at ../../Source/WebCore/html/parser/HTMLScriptRunner.cpp:123
#20 0x00002b947ac8fb91 in WebCore::HTMLScriptRunner::executeParsingBlockingScripts (this=0xc328f0) at ../../Source/WebCore/html/parser/HTMLScriptRunner.cpp:195
#21 0x00002b947ac8fcf4 in WebCore::HTMLScriptRunner::executeScriptsWaitingForLoad (this=0xc328f0, cachedScript=0x2b948c00a0c0) at ../../Source/WebCore/html/parser/HTMLScriptRunner.cpp:206
#22 0x00002b947ac849da in WebCore::HTMLDocumentParser::notifyFinished (this=0xcad8d0, cachedResource=0x2b948c00a0c0) at ../../Source/WebCore/html/parser/HTMLDocumentParser.cpp:496
#23 0x00002b947ad89c52 in WebCore::CachedScript::checkNotify (this=0x2b948c00a0c0) at ../../Source/WebCore/loader/cache/CachedScript.cpp:112
#24 0x00002b947ad89bd7 in WebCore::CachedScript::data (this=0x2b948c00a0c0, data=..., allDataReceived=true) at ../../Source/WebCore/loader/cache/CachedScript.cpp:102
#25 0x00002b947ad88595 in WebCore::CachedResourceRequest::didFinishLoading (this=0x2b948c009670, loader=0x2b948c055460) at ../../Source/WebCore/loader/cache/CachedResourceRequest.cpp:160
#26 0x00002b947aded112 in WebCore::SubresourceLoader::didFinishLoading (this=0x2b948c055460, finishTime=0) at ../../Source/WebCore/loader/SubresourceLoader.cpp:181
#27 0x00002b947ade432d in WebCore::ResourceLoader::didFinishLoading (this=0x2b948c055460, finishTime=0) at ../../Source/WebCore/loader/ResourceLoader.cpp:436
#28 0x00002b947a740670 in WebCore::readCallback (source=0x129c4c0, asyncResult=0xcb1520, data=0x0) at ../../Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:781
#29 0x00002b947e77bf65 in async_ready_callback_wrapper (source_object=0x129c4c0, res=0xcb1520, user_data=0x0) at /tmp/buildd/glib2.0-2.27.91/./gio/ginputstream.c:470
#30 0x00002b947e78d628 in complete_in_idle_cb_for_thread (_data=<value optimized out>) at /tmp/buildd/glib2.0-2.27.91/./gio/gsimpleasyncresult.c:812
#31 0x00002b947f2f4362 in g_main_dispatch (context=0xc0e760) at /tmp/buildd/glib2.0-2.27.91/./glib/gmain.c:2440
#32 g_main_context_dispatch (context=0xc0e760) at /tmp/buildd/glib2.0-2.27.91/./glib/gmain.c:3013
#33 0x00002b947f2f8a28 in g_main_context_iterate (context=0xc0e760, block=<value optimized out>, dispatch=<value optimized out>, self=<value optimized out>) at /tmp/buildd/glib2.0-2.27.91/./glib/gmain.c:3091
#34 0x00002b947f2f8f35 in g_main_loop_run (loop=0xd08be0) at /tmp/buildd/glib2.0-2.27.91/./glib/gmain.c:3299
#35 0x00002b947d267657 in gtk_main () from /usr/lib/libgtk-x11-2.0.so.0
#36 0x000000000041e5ad in runTest (testPathOrURL=...) at ../../Tools/DumpRenderTree/gtk/DumpRenderTree.cpp:679
#37 0x000000000041dc3f in runTestingServerLoop () at ../../Tools/DumpRenderTree/gtk/DumpRenderTree.cpp:489
#38 0x000000000041fd24 in main (argc=2, argv=0x7fff16b06688) at ../../Tools/DumpRenderTree/gtk/DumpRenderTree.cpp:1143