Bug 118918 – Eager stack trace for error objects.

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Chris Curtis]

The .stack property is not built at the creation of the error object.

[Chris Curtis]

Created attachment 207143 [details]
patch for initial review

inspector/console/console-format.html test will still fail.

[Build Bot]

Comment on attachment 207143 [details]
patch for initial review

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

New failing tests:
inspector/console/console-format.html

[Build Bot]

Created attachment 207151 [details]
Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2

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

[Build Bot]

Comment on attachment 207143 [details]
patch for initial review

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

New failing tests:
inspector/console/console-format.html

[Build Bot]

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

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

[Geoffrey Garen]

Comment on attachment 207143 [details]
patch for initial review

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

> Source/JavaScriptCore/ChangeLog:24
> +        * dfg/DFGJITCompiler.cpp:
> +        (JSC::DFG::JITCompiler::compileFunction):

Need a comment here about what the bug was in this code.

> Source/JavaScriptCore/runtime/ErrorInstance.h:70
> +            if (stackTrace.isEmpty())
> +                return;

Even if there's no stack trace, shouldn't we still produce a .stack property -- just with the empty string? i.e., shouldn't the expression ("stack" in error) always be true? What do other browser do here? Can you add a test for this?

> Source/JavaScriptCore/runtime/ErrorInstance.h:81
> +            if (!builder.isEmpty())
> +                putDirect(vm, vm.propertyNames->stack, jsString(&vm, builder.toString()), ReadOnly | DontDelete);

Ditto.

> Source/JavaScriptCore/runtime/NativeErrorConstructor.cpp:69
> +    NativeCallFrameTracer(&exec->vm(), exec->callerFrame());

I see why you did this, but it's a bit surprising.

This is a sort of spooky action at a distance: providing the wrong data (removing the top item from the stack) because you happen to know that it will produce the right result later (JS backtrace with top frame omitted). This makes it hard to reason about different pieces of the software separately and with modularity.

One way this could go wrong in future is if another coder adds code beneath constructWithNativeErrorConstructor or callNativeErrorConstructor that can throw an exception. That exception will be missing the top frame in its backtrace.

I think it would be better for these functions to pass the ErrorInstance constructor an argument specifying "please skip the first item in the stack when filling in your .stack property". An enum is good for this.

ErrorInstance code:

enum BacktraceMode { Full, ExcludeTopStackFrame };
static ErrorInstance* create(ExecState* exec, Structure* structure, JSValue message, BacktraceMode = Full);

constructWithNativeErrorConstructor code:

return JSValue::encode(ErrorInstance::create(exec, errorStructure, message, ExcludeTopStackFrame)); // Match other browsers by excluding the Error constructor from "(new Error()).stack".

> LayoutTests/ChangeLog:21
> +        * fast/js/exception-line-number-expected.txt:
> +        * fast/js/exception-properties-expected.txt:
> +        * fast/js/line-column-numbers-expected.txt:
> +        * fast/js/script-tests/exception-line-number.js:
> +        (window.onerror):
> +        * fast/js/script-tests/exception-properties.js:
> +        * fast/js/script-tests/stack-at-creation-for-error-objects.js: Added.
> +        (log):
> +        (checkStack):
> +        * fast/js/stack-at-creation-for-error-objects-expected.txt: Added.
> +        * fast/js/stack-at-creation-for-error-objects.html: Added.
> +        * fast/js/stack-trace-expected.txt:

You should comment about why these line number changes are an improvement. You don't need to elaborate on each case, but one example would be nice.

> LayoutTests/fast/js/script-tests/stack-at-creation-for-error-objects.js:21
> +checkStack(Error());
> +checkStack(EvalError());
> +checkStack(RangeError());
> +checkStack(ReferenceError());
> +checkStack(SyntaxError());
> +checkStack(TypeError());
> +checkStack(URIError());

Even though you can create these objects just by calling their constructors, it's more idiomatic to use 'new': new Error(), etc.

[Chris Curtis]

(In reply to comment #6)
> (From update of attachment 207143 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=207143&action=review
> 
> > Source/JavaScriptCore/runtime/ErrorInstance.h:70
> > +            if (stackTrace.isEmpty())
> > +                return;
> 
> Even if there's no stack trace, shouldn't we still produce a .stack property -- just with the empty string? i.e., shouldn't the expression ("stack" in error) always be true? What do other browser do here? Can you add a test for this?
> 
> > Source/JavaScriptCore/runtime/ErrorInstance.h:81
> > +            if (!builder.isEmpty())
> > +                putDirect(vm, vm.propertyNames->stack, jsString(&vm, builder.toString()), ReadOnly | DontDelete);
> 

The reasoning for these checks is to make sure that the Error prototype does not receive the stack property. There should not be a case where an error object is being created and there is no stack trace. That would mean that there was an error before evaluating the code at global scope. 

Although the first check was redundant, so I removed it. If you wanted to let the prototypes have the .stack property we could do that though.

[Chris Curtis]

Created attachment 207291 [details]
patch2

[Michael Saboff]

Comment on attachment 207291 [details]
patch2

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

r=me with suggested changes

> Source/JavaScriptCore/ChangeLog:15
> +        the stack trace to be correct without modifying the state of

Incomplete comment.

> Source/JavaScriptCore/interpreter/Interpreter.cpp:626
> +void Interpreter::getStackTrace(VM* vm, Vector<StackFrame>& results, size_t maxStackSize, BacktraceMode isFullStackTrace)

isFullStackTrace sounds like a boolean.  If the enum name change below is done, I'd call the added param backtraceType.

> Source/JavaScriptCore/interpreter/Interpreter.cpp:630
> +        callFrame= callFrame->callerFrame();

Add space before '='

> Source/JavaScriptCore/interpreter/Interpreter.cpp:631
> +    if (!callFrame || callFrame == CallFrame::noCaller())

This check should be done before removing the top frame as well as here.

> Source/JavaScriptCore/interpreter/Interpreter.h:65
> +    enum BacktraceMode {
> +        Full,
> +        ExcludeTopStackFrame
> +    };

How about calling the enum BacktraceType and the values IncludeTopStackFrame and ExcludeTopStackFrame.

> Source/JavaScriptCore/runtime/ErrorInstance.h:41
> +        static ErrorInstance* create(VM& vm, Structure* structure, const String& message, BacktraceMode isFullBackTrace = Full)

ditto

> Source/JavaScriptCore/runtime/ErrorInstance.h:48
> +        static ErrorInstance* create(ExecState* exec, Structure* structure, JSValue message, BacktraceMode isFullStackTrace = Full)

ditto

> Source/JavaScriptCore/runtime/ErrorInstance.h:60
> +        void finishCreation(VM& vm, const String& message, BacktraceMode isFullStackTrace = Full)

ditto

> Source/JavaScriptCore/runtime/ErrorInstance.h:73
> +                builder.append(String(stackTrace[i].toString(vm.dynamicGlobalObject->globalExec()).impl()));

I think this should be builder.append(stackTrace[i].toString(vm.dynamicGlobalObject->globalExec()));

[Geoffrey Garen]

> > > Source/JavaScriptCore/runtime/ErrorInstance.h:81
> > > +            if (!builder.isEmpty())
> > > +                putDirect(vm, vm.propertyNames->stack, jsString(&vm, builder.toString()), ReadOnly | DontDelete);
> > 
> 
> The reasoning for these checks is to make sure that the Error prototype does not receive the stack property. There should not be a case where an error object is being created and there is no stack trace. That would mean that there was an error before evaluating the code at global scope. 
> 
> Although the first check was redundant, so I removed it. If you wanted to let the prototypes have the .stack property we could do that though.

putDirect doesn't assign to the prototype.

[Geoffrey Garen]

> putDirect doesn't assign to the prototype.

But maybe you mean that you're using the lack of a backtrace to detect the fact that the ErrorInstance being created is the ErrorPrototype? This is also a mild case of spooky action at a distance: We can't tell the difference between not wanting a stack trace (OK), and wanting one but not having one (bug).

I think a clearer way to communicate this is to add a BacktraceType named something like "NoBacktrace", and have the ErrorPrototype use it.

[Chris Curtis]

Created attachment 207349 [details]
patch3

[Geoffrey Garen]

Comment on attachment 207349 [details]
patch3

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

> Source/JavaScriptCore/ChangeLog:9
> +        For better error information, we want to be able to have the error objects have the 
> +        .stack property at creation.

Let's shorten "we want to be able to have the error objects" to "we want error objects to".

The goal here isn't really better information -- although you discovered that's a nice side effect. The goal is to fill in an API that other browsers provide, which allows you to get the current stack trace without having to throw an exception. You should clarify this goal in your ChangeLog.

> Source/JavaScriptCore/ChangeLog:15
> +        For all error objects that the user explicitly creates, the topCallFrame is at a 
> +        new frame created to handle the user's call. In this case though, the error 
> +        object needs the caller's frame to create the stack trace correctly. An enum was 
> +        created to notify the object that it needs to adjust its stackTrace. This allows 
> +        the stack trace to be correct without modifying the state of topCallFrame.

I would move this comment next to constructWithErrorConstructor and callErrorConstructor. It helps clarify that you're talking about those specific functions.

> Source/JavaScriptCore/interpreter/Interpreter.cpp:626
> +void Interpreter::getStackTrace(VM* vm, Vector<StackFrame>& results, size_t maxStackSize, BacktraceMode backTraceType)

It's bad to give one thing two names. This should either be "BacktraceMode backtraceMode" or "BacktraceType backtraceType" -- you can pick which name you prefer.

> Source/JavaScriptCore/interpreter/Interpreter.cpp:675
> +        if (asObject(error)->hasProperty(callFrame, vm->propertyNames->stack) && vm->exceptionStack().size())

Why the check for vm->exceptionStack().size() here? Are there cases where we initially create an ErrorInstance with a .stack property that's the empty string, even though an exception has been thrown? If so, I think this warrants a comment explaining the case you're guarding against.

> Source/JavaScriptCore/runtime/ErrorInstance.h:72
> +            vm.exceptionStack() = RefCountedArray<StackFrame>(stackTrace);

Why do we do this?  It seems wrong to set the exceptionStack value when an exception hasn't been thrown. Won't this make it look like an exception has been thrown?

> Source/JavaScriptCore/runtime/ErrorInstance.h:80
> +            StringBuilder builder;
> +            for (unsigned i = 0; i < stackTrace.size(); i++) {
> +                builder.append(String(stackTrace[i].toString(vm.dynamicGlobalObject->globalExec())));
> +                if (i != stackTrace.size() - 1)
> +                    builder.append('\n');
> +            }
> +            putDirect(vm, vm.propertyNames->stack, jsString(&vm, builder.toString()), ReadOnly | DontDelete);

There's enough code here that this function should move to the .cpp file and not be inlined anymore.

> LayoutTests/ChangeLog:15
> +        The following test was modified to remove the error element. Prior to this patch 

"element" here should be "object".

> LayoutTests/ChangeLog:16
> +        the error object would evaluate null because there was no stack property. The stack 

I'm not sure what "evaluate null" means here. Are you saying it would take the value null and try to evaluate it as a script?

> LayoutTests/ChangeLog:17
> +        trace included a file path specific to the machine that is running the test, The 

"is" should be "was". "." should be ",".

> LayoutTests/fast/js/script-tests/stack-at-creation-for-error-objects.js:5
> +'This test checks that there is a stack element at creation of each error object.'

"element" should be "property" here.

> LayoutTests/fast/js/script-tests/stack-at-creation-for-error-objects.js:10
> +        testFailed(errorObject + " did not have a .stack element at creation time.");

Ditto.

Also, can we test for something more than just being defined? You can use typeof and .length to verify that we have a non-empty string. That's a little more testing.

[Chris Curtis]

Created attachment 207475 [details]
patch 4

I reworked how exception objects and user created objects were treated at creation time. It made the code much cleaner, otherwise the vm exception stack and the error object stack were difficult to keep in sync.

[Chris Curtis]

Created attachment 207497 [details]
patch 5

[WebKit Commit Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast/js/exception-properties-expected.txt', u'LayoutTests/fast/js/line-column-numbers-expected.txt', u'LayoutTests/fast/js/script-tests/exception-properties.js', u'LayoutTests/fast/js/script-tests/stack-at-creation-for-error-objects.js', u'LayoutTests/fast/js/stack-at-creation-for-error-objects-expected.txt', u'LayoutTests/fast/js/stack-at-creation-for-error-objects.html', u'LayoutTests/fast/js/stack-trace-expected.txt', u'LayoutTests/inspector/console/console-format-expected.txt', u'LayoutTests/inspector/console/console-format.html', u'Source/JavaScriptCore/ChangeLog', u'Source/JavaScriptCore/interpreter/Interpreter.cpp', u'Source/JavaScriptCore/interpreter/Interpreter.h', u'Source/JavaScriptCore/runtime/ErrorConstructor.cpp', u'Source/JavaScriptCore/runtime/ErrorInstance.cpp', u'Source/JavaScriptCore/runtime/ErrorInstance.h', u'Source/JavaScriptCore/runtime/ErrorPrototype.cpp', u'Source/JavaScriptCore/runtime/NativeErrorConstructor.cpp']" exit_code: 1
Source/JavaScriptCore/interpreter/Interpreter.cpp:605:  Missing space before {  [whitespace/braces] [5]
Total errors found: 1 in 18 files


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

[Chris Curtis]

Created attachment 207535 [details]
patch 6

Had to update my code after the merge.

[Chris Curtis]

Created attachment 207539 [details]
patch 6.1

When merging I didnt clean my version of svn. There was extra code that shouldnt have been there.

[Chris Curtis]

Created attachment 207540 [details]
patch 6.2

needed to include a test file

[Geoffrey Garen]

Comment on attachment 207539 [details]
patch 6.1

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

> Source/JavaScriptCore/ChangeLog:10
> +        Chrome and Firefox give error objects the stack property and we wanted to match
> +        that functionality. This allows developers to see the stack without throwing an object.
> +

Much improved.

> Source/JavaScriptCore/ChangeLog:22
> +        1. User created errors, Only the stack property is set.

"," should be ":".

"User created errors" isn't quite the right way to describe this category. A user can create an error an then throw it. In that case, we'll have a user-created error, but we won't have this category.

I think the best way to distinguish is "Error object that is not thrown as an exception" and "Error object that is thrown as an exception".

> Source/JavaScriptCore/ChangeLog:23
> +        2. Exception created errors, the stack property and the vm->exception is set. Both

"," should be ":", and "the" should be "The" to match how you capitalized "Only".

"is" should be "are".

> Source/JavaScriptCore/ChangeLog:31
> +        (JSC::Interpreter::addStackTraceIfNecessary):
> +        When an error object is created by the user the vm->exceptionStack is not set.
> +        If the user throws this error object later the stack that is in the error object 
> +        may not be the correct stack for the throw, so when we set the vm->exception stack,
> +        the stack property on the error object is set as well.

Great explanation.

> Source/JavaScriptCore/interpreter/Interpreter.cpp:531
> +void Interpreter::getStackTrace(Vector<StackFrame>& results, size_t maxStackSize, StackTraceType StackTraceType)

Types get their first letters capitalized, but names do not. This should be "StackTraceType stackTraceType".

> Source/JavaScriptCore/interpreter/Interpreter.cpp:575
> +    JSGlobalObject* globalObject = 0;
> +
> +    if (error) {
> +        if (isTerminatedExecutionException(*error))
> +            globalObject = vm->dynamicGlobalObject;
> +        else
> +            globalObject = asObject(*error)->globalObject();
> +    } else
> +        globalObject = vm->dynamicGlobalObject;

This looks a little bit brittle: There's no guarantee that vm->dynamicGlobalObject is not null. Let's have our callers pass us an ExecState* instead. (This means we can remove the VM* argument, too, since we can do exec->vm().)

> Source/JavaScriptCore/interpreter/Interpreter.cpp:595
> +JSString* Interpreter::getStackTraceForErrorObjects(VM* vm, StackTraceType StackTraceType, bool exceptionError)
> +{    
> +    Vector<StackFrame> stackTrace;
> +    vm->interpreter->getStackTrace(stackTrace, std::numeric_limits<size_t>::max(), StackTraceType);
> +    if (exceptionError)
> +        vm->exceptionStack() = RefCountedArray<StackFrame>(stackTrace);
> +
> +    return vm->interpreter->stackTraceAsString(vm, stackTrace);
> +}

This is too tight coupling between the Error object and the Interpreter. I'll try to suggest something better below.

> Source/JavaScriptCore/interpreter/Interpreter.cpp:602
> +    // The error object and the vm need to have a stack trace to return early. If either is not set,

This comment just restates the code below. You should remove it.

> Source/JavaScriptCore/interpreter/Interpreter.cpp:603
> +    // both are set to have the information match for preperation for the exceoption throw

This comment doesn't explain why we want the information to match. The key detail is that the Error might have gotten a distinct .stack property when it was created.

What the reader really wants to know is: What are the conditions I should be aware of and concerned about, and when can they arise?

I think there are two conditions we care about:

(1) No stack has been recorded: we need to record one now, in both the VM and the thrown object.

(2) The error object recorded a stack at creation time, but wasn't thrown until later: its stack needs to be updated.

I would write that like this:

if (vm->exceptionStack().size()) {
    ASSERT(!error.isObject() || asObject(error)->hasProperty(callFrame, vm->propertyNames->stack));
    return;
}

....

// Note: 'error' might already have a stack property if it was created by the user (e.g. "new Error"). The stack
// now, as the error is thrown, might be different from the stack when it was created, so we overwrite it with
// the current stack unconditionally.
asObject(error)->putDirect(....);

....

> Source/JavaScriptCore/interpreter/Interpreter.h:66
> +        StackTraceForUserError,

"UserError" isn't a great way to describe what we want. It only makes sense if you know that by "User" you mean "one of the global Error constructors, used outside the context of a throw instruction" and if you also know that those constructors choose to exclude themselves from the stack trace they provide. We don't want the Interpreter class to have to know all these internal implementation details of other parts of the engine. We just want it to do what it does best: give us a stack trace. So, we should ask for exactly what we want. That means something like "ExcludeTopCallFrame".

> Source/JavaScriptCore/runtime/ErrorConstructor.cpp:58
>  {
>      JSValue message = exec->argumentCount() ? exec->argument(0) : jsUndefined();
>      Structure* errorStructure = asInternalFunction(exec->callee())->globalObject()->errorStructure();
> -    return JSValue::encode(ErrorInstance::create(exec, errorStructure, message));
> +    return JSValue::encode(ErrorInstance::create(exec, errorStructure, message, StackTraceForUserError));
>  }
>  
>  ConstructType ErrorConstructor::getConstructData(JSCell*, ConstructData& constructData)

This should just call getStackTrace, specifying that it wants to exclude the top call frame, and pass the result to ErrorInstance::create. Alternatively, it could call getStackTrace and then call removeLast() on the result. That might be even clearer.

> Source/JavaScriptCore/runtime/ErrorConstructor.cpp:68
> +    return JSValue::encode(ErrorInstance::create(exec, errorStructure, message, StackTraceForUserError));

Ditto.

> Source/JavaScriptCore/runtime/ErrorInstance.cpp:59
> +void ErrorInstance::finishCreation(VM& vm, const String& message, StackTraceType StackTraceType)
> +{
> +    Base::finishCreation(vm);
> +    ASSERT(inherits(&s_info));
> +    if (!message.isNull())
> +        putDirect(vm, vm.propertyNames->message, jsString(&vm, message), DontEnum);
> +
> +    if (StackTraceType == NoStackTrace)
> +        return;
> +    
> +    // If the error object was user created, StackTraceType will excludeTopCallFrame. Otherwise it is an exception error.
> +    // We want to set the vm->exceptionstack becasue this will limit the amount of times that the stack Trace has to be retrieved. 
> +    
> +    if (StackTraceType == StackTraceForUserError)
> +        putDirect(vm, vm.propertyNames->stack, vm.interpreter->getStackTraceForErrorObjects(&vm, StackTraceType), ReadOnly | DontDelete);
> +    else {
> +        bool exceptionError = true;
> +        putDirect(vm, vm.propertyNames->stack, vm.interpreter->getStackTraceForErrorObjects(&vm, StackTraceType, exceptionError), ReadOnly | DontDelete);
> +    }
> +    
> +}

Looking at how this shook out, I think this is too tight coupling between creating an error object and indicating to the VM that an exception has been thrown. "new Error" and "throw new Error" are very different things, and we shouldn't confuse them in our implementation.

I think the right way to do this is for the ErrorInstance to take a reference to a stack trace as an argument, and if it's non-null, set it as the stack property. It's up to the caller to decide, when it produces the stack trace, where it's doing so for information, or as part of throwing an exception.

> Source/JavaScriptCore/runtime/ErrorPrototype.cpp:59
> +    Base::finishCreation(exec->vm(), "", NoStackTrace);

This should be "Base::finishCreation(exec->vm(), "", Vector<StackFrame>());

> Source/JavaScriptCore/runtime/NativeErrorConstructor.cpp:60
> -    return JSValue::encode(ErrorInstance::create(exec, errorStructure, message));
> +    return JSValue::encode(ErrorInstance::create(exec, errorStructure, message, StackTraceForUserError));
>  }
>  
>  ConstructType NativeErrorConstructor::getConstructData(JSCell*, ConstructData& constructData)

Same as ErrorInstance::create above.

> Source/JavaScriptCore/runtime/NativeErrorConstructor.cpp:70
> +    return JSValue::encode(ErrorInstance::create(exec, errorStructure, message, StackTraceForUserError));

Same as ErrorInstance::create above.

[Chris Curtis]

Created attachment 207671 [details]
patch 7

[Geoffrey Garen]

Comment on attachment 207671 [details]
patch 7

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

r=me

> Source/JavaScriptCore/interpreter/Interpreter.cpp:571
> +    JSGlobalObject* globalObject = 0;
>  
> +    if (error) {
> +        if (isTerminatedExecutionException(*error))
> +            globalObject = vm->dynamicGlobalObject;
> +        else
> +            globalObject = asObject(*error)->globalObject();
> +    } else
> +        globalObject = vm->dynamicGlobalObject;

Sorry, I should have been clearer here: now that you've threaded "exec" into this function, you can remove all this code. This code is only used to produce globalObject->globalExec(). You can use "exec" instead, and delete this code.

Please fix this in a follow-up patch.

> Source/JavaScriptCore/interpreter/Interpreter.cpp:590
> -        if (asObject(error)->hasProperty(callFrame, vm->propertyNames->stack))
> +    if (vm->exceptionStack().size()) {
> +        if (!error.isObject() || asObject(error)->hasProperty(callFrame, vm->propertyNames->stack))
>              return;

Please file a follow-up bug about the ASSERT you saw firing in this case.

[WebKit Commit Bot]

Comment on attachment 207671 [details]
patch 7

Rejecting attachment 207671 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.appspot.com', '--bot-id=webkit-cq-02', 'validate-changelog', '--check-oops', '--non-interactive', 207671, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

ChangeLog entry in LayoutTests/ChangeLog contains OOPS!.

Full output: http://webkit-queues.appspot.com/results/1295145

[Chris Curtis]

Created attachment 207685 [details]
patch 7 followup

[Geoffrey Garen]

Comment on attachment 207685 [details]
patch 7 followup

r=me

[WebKit Commit Bot]

Comment on attachment 207685 [details]
patch 7 followup

Clearing flags on attachment: 207685

Committed r153457: <http://trac.webkit.org/changeset/153457>