Bug 94433 – Add call stacks to Content Security Policy checks when relevant.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Mike West]

I've been reliably informed by pfeldman that generating call stacks is a good way of ensuring that there's enough detail in the console message to figure out what's going on. This is particularly true for the various bits of inline code that CSP might block when they're injected via JavaScript. We still won't be able to point directly to them via a line number, but the call stack should give developers enough info to determine what they're doing that's causing an error.

Whenever relevant, we should generate a call stack, and pipe it through the CSP check in order to get it into the log if it's relevant.

[Mike West]

Created attachment 159304 [details]
Patch

[Mike West]

Hi Adam! This patch adds moar detail to CSP's console logs by generating call stacks, and passing them down to the CSP object. I think it's pretty straightforward from the CSP side, though I'd love to have tests.

Speaking of tests, and the console, hello Pavel! Do you have a recommendation regarding testing for this patch? I've added call stacks to a variety of console warnings, but DRT doesn't currently output anything that would identify which logs have call stacks, and which don't. Is there a mechanism I could use to make that determination without modifying DRT (and then rebaselining every test that uses the console? :) ).

Thanks to you both!

[Mike West]

Actually adding Pavel to CC this time.

[Gyuyoung Kim]

Comment on attachment 159304 [details]
Patch

Attachment 159304 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13527742

[Gustavo Noronha (kov)]

Comment on attachment 159304 [details]
Patch

Attachment 159304 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/13531719

[Early Warning System Bot]

Comment on attachment 159304 [details]
Patch

Attachment 159304 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/13538441

[Build Bot]

Comment on attachment 159304 [details]
Patch

Attachment 159304 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13531721

[Early Warning System Bot]

Comment on attachment 159304 [details]
Patch

Attachment 159304 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/13539332

[WebKit Review Bot]

Comment on attachment 159304 [details]
Patch

Attachment 159304 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13528736

New failing tests:
http/tests/inspector/appcache/appcache-iframe-manifests.html
http/tests/inspector/injected-script-for-origin.html
http/tests/inspector/extensions-network-redirect.html
http/tests/inspector/change-iframe-src.html
http/tests/inspector/modify-cross-domain-rule.html
http/tests/inspector/resource-har-conversion.html
http/tests/inspector/console-resource-errors.html
http/tests/inspector/appcache/appcache-manifest-with-non-existing-file.html
http/tests/inspector/extensions-headers.html
http/tests/inspector-enabled/injected-script-discard.html
http/tests/inspector/console-cross-origin-iframe-logging.html
http/tests/inspector-enabled/contentSecurityPolicy-blocks-setTimeout.html
http/tests/inspector/extensions-useragent.html
http/tests/inspector/console-cd-completions.html
http/tests/inspector/compiler-script-mapping.html
http/tests/inspector-enabled/contentSecurityPolicy-blocks-setInterval.html
http/tests/inspector/compiler-source-mapping-debug.html
http/tests/inspector/web-socket-frame-error.html
http/tests/inspector/console-xhr-logging.html
http/tests/inspector/console-xhr-logging-async.html
http/tests/inspector/console-cd.html
http/tests/inspector/resource-har-headers.html
http/tests/inspector/network-preflight-options.html
http/tests/inspector/inspect-iframe-from-different-domain.html
http/tests/inspector-enabled/console-log-before-frame-navigation.html
http/tests/inspector-enabled/dom-storage-open.html
http/tests/inspector/resource-parameters.html
http/tests/inspector-enabled/database-open.html

[WebKit Review Bot]

Created attachment 159308 [details]
Archive of layout-test-results from gce-cr-linux-04

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-04  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[Adam Barth]

Looks like you've got some test failures to work through...

[Mike West]

(In reply to comment #11)
> Looks like you've got some test failures to work through...

Hrm. Well, at least the style bot passed. :)

Back to the drawing board!

[Mike West]

Created attachment 160736 [details]
Patch

[Mike West]

(In reply to comment #12)
> (In reply to comment #11)
> > Looks like you've got some test failures to work through...
> 
> Hrm. Well, at least the style bot passed. :)
> 
> Back to the drawing board!

The drawing board was a good place to be. While working through a separate bug, I realized that we can really generate stack traces from wherever we like. This means that we can actually just generate the trace directly before logging a message: there's no reason to generate a stack trace _every time_ we check 'allowEval'.

This also enables us to get stack traces from any JavaScript-driven CSP violation. Resource injection now traces back up to the JS code that does the injection, which is nice indeed.

Let's hope the bots like it as much as I do.

WDYT, Adam/Pavel?

[Build Bot]

Comment on attachment 160736 [details]
Patch

Attachment 160736 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13609426

[Early Warning System Bot]

Comment on attachment 160736 [details]
Patch

Attachment 160736 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/13609434

[Early Warning System Bot]

Comment on attachment 160736 [details]
Patch

Attachment 160736 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/13615255

[Mike West]

Created attachment 160749 [details]
JSC fix. :(

[Adam Barth]

Cool!

[WebKit Review Bot]

Comment on attachment 160749 [details]
JSC fix. :(

Attachment 160749 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13612330

New failing tests:
http/tests/inspector/csp-inline-warning-contains-stacktrace.html
http/tests/inspector/csp-injected-content-warning-contains-stacktrace.html

[WebKit Review Bot]

Created attachment 160766 [details]
Archive of layout-test-results from gce-cr-linux-01

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-01  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[Mike West]

(In reply to comment #20)
> (From update of attachment 160749 [details])
> Attachment 160749 [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://queues.webkit.org/results/13612330
> 
> New failing tests:
> http/tests/inspector/csp-inline-warning-contains-stacktrace.html
> http/tests/inspector/csp-injected-content-warning-contains-stacktrace.html

Hey Pavel, these tests are giving different column numbers for the inspector test code on the EWS bots than they are on my local machine. Is that something you've seen before?

If the exact lines of the inspector-test/debugger-test are somehow flakey, should I adjust the test to exclude them?

[Mike West]

(In reply to comment #22)
> If the exact lines of the inspector-test/debugger-test are somehow flakey, should I adjust the test to exclude them?

Actually, it looks like 'debugger-test.js' does exactly that: internal scripts are labeled as "(internal script)" with line number "(line number)". I'll adjust my tests accordingly.

[Mike West]

Created attachment 160774 [details]
Happy bots?

[Mike West]

Comment on attachment 160774 [details]
Happy bots?

Happy bots! CQ? :)

[Kentaro Hara]

Comment on attachment 160774 [details]
Happy bots?

oh, please manually replace "Reviewed by NOBODY (OOPS)" with "Reviewed by Adam Barth".

[Mike West]

(In reply to comment #26)
> (From update of attachment 160774 [details])
> oh, please manually replace "Reviewed by NOBODY (OOPS)" with "Reviewed by Adam Barth".

Hrm. I thought the CQ took care of that?

I'll add it in right now.. thanks for hopping on the CQ request so quickly! :)

[Kentaro Hara]

(In reply to comment #27)
> Hrm. I thought the CQ took care of that?

The CQ takes care of it if you keep the r+ you obtained. Now you've cleared r+ you obtained, you need to manually replace it.

[Mike West]

Created attachment 160905 [details]
Patch

[Kentaro Hara]

Comment on attachment 160905 [details]
Patch

ok

[Mike West]

(In reply to comment #28)
> (In reply to comment #27)
> > Hrm. I thought the CQ took care of that?
> 
> The CQ takes care of it if you keep the r+ you obtained. Now you've cleared r+ you obtained, you need to manually replace it.

Understood. Thanks!

[WebKit Review Bot]

Comment on attachment 160905 [details]
Patch

Clearing flags on attachment: 160905

Committed r126852: <http://trac.webkit.org/changeset/126852>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

(In reply to comment #32)
> (From update of attachment 160905 [details])
> Clearing flags on attachment: 160905
> 
> Committed r126852: <http://trac.webkit.org/changeset/126852>

It broke many tests - https://bugs.webkit.org/show_bug.cgi?id=95176
Could you check it, please?

[Kentaro Hara]

(In reply to comment #34)
> It broke many tests - https://bugs.webkit.org/show_bug.cgi?id=95176
> Could you check it, please?

Looks like Mike is not around now. Let me roll out the patch.

[Kentaro Hara]

Reverted r126852 for reason:

broke qt and mac tests

Committed r126854: <http://trac.webkit.org/changeset/126854>

[Mike West]

(In reply to comment #36)
> Reverted r126852 for reason:
> 
> broke qt and mac tests
> 
> Committed r126854: <http://trac.webkit.org/changeset/126854>

It looks like this is simply the output that JSC is generating; the results I here here on WK2/Lion have the "native code" stack traces, while the results I get for Chromium are in line with what this patch contains.

I'll reset the baseline test to these new values, and override it with a platform specific Chromium test containing the old.

I'd like Pavel to take a look at it before trying again though...

[Mike West]

Created attachment 161022 [details]
Chromium vs Everything Else.

[Mike West]

(In reply to comment #38)
> Created an attachment (id=161022) [details]
> Chromium vs Everything Else.

Hey Pavel: not a stunningly high-priority bug, but when you have some time, I'd appreciate you evaluating the V8 vs JSC test expectations in the attached patch.

Thanks!

[Mike West]

(In reply to comment #39)
> (In reply to comment #38)
> > Created an attachment (id=161022) [details] [details]
> > Chromium vs Everything Else.
> 
> Hey Pavel: not a stunningly high-priority bug, but when you have some time, I'd appreciate you evaluating the V8 vs JSC test expectations in the attached patch.
> 
> Thanks!

Friendly ping, Pavel. I know I said low priority, but I'd love to see this land in time for M23 if possible. Do you think you'll have time this week?

Thanks!

[Mike West]

Comment on attachment 161022 [details]
Chromium vs Everything Else.

(resetting the r? flag, I don't want to land this again without someone signing off. :) )

[Adam Barth]

Mike, are you waiting for Pavel here, or can I help?

[Pavel Feldman]

(In reply to comment #42)
> Mike, are you waiting for Pavel here, or can I help?

Hey, sorry, I'm on vacations. Please ping [e-mail] or [e-mail] for assistance.

[Mike West]

Thanks for the ping Adam, but I think your work here is done! It's just web inspector stuff from here on out. :)

Thanks Pavel, sorry, I didn't realize you were out of office.

Hi Vsevolod and Yury! Would one of you mind taking a look a this patch? I landed it a week or two ago, and had to roll it back because of differences between JSC and V8's generated stack traces. I'd appreciate you reviewing the platform-specific tests to make sure that the differences are expected, and not a result of something weird I've done.

[Yury Semikhatsky]

Comment on attachment 161022 [details]
Chromium vs Everything Else.

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

r=me

> Source/WebCore/ChangeLog:48
> +            stack trace just before logging it, and only pass it to

Nice.

> LayoutTests/http/tests/inspector/resources/csp-test.js:4
> +    var messageCount = 0;

You don't need this counter. Just drop the second argument in the call to InspectorTest.addConsoleSniffer above.

[Yury Semikhatsky]

Comment on attachment 161022 [details]
Chromium vs Everything Else.

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

> LayoutTests/http/tests/inspector-enabled/contentSecurityPolicy-blocks-setInterval-expected.txt:7
> + [native code]:1

As Vsevolod pointed out this is a regression as now we don't have exact location for JSC. I guess that JSMainThreadExecState::currentState returns 0 in this case because it doesn't use JSMainThreadExecState::call. This can be fixed by adding 
JSMainThreadExecState currentState(exec);
to ScheduledAction::create.

r- for this

[Yury Semikhatsky]

(In reply to comment #46)
> (From update of attachment 161022 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=161022&action=review
> 
> > LayoutTests/http/tests/inspector-enabled/contentSecurityPolicy-blocks-setInterval-expected.txt:7
> > + [native code]:1
> 
> As Vsevolod pointed out this is a regression as now we don't have exact location for JSC. I guess that JSMainThreadExecState::currentState returns 0 in this case because it doesn't use JSMainThreadExecState::call. This can be fixed by adding 
> JSMainThreadExecState currentState(exec);
> to ScheduledAction::create.
> 
> r- for this

On second thought, we cannot just put JSMainThreadExecState currentState(exec); into ScheduledAction::create as it wouldn't work for Workers. They also can call setTimeout/Interval which would lead to modification of JSMainThreadExecState::s_mainThreadState from wrong thread.

[Mike West]

(In reply to comment #47)
> (In reply to comment #46)
> > (From update of attachment 161022 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=161022&action=review
> > 
> > > LayoutTests/http/tests/inspector-enabled/contentSecurityPolicy-blocks-setInterval-expected.txt:7
> > > + [native code]:1
> > 
> > As Vsevolod pointed out this is a regression as now we don't have exact location for JSC. I guess that JSMainThreadExecState::currentState returns 0 in this case because it doesn't use JSMainThreadExecState::call. This can be fixed by adding 
> > JSMainThreadExecState currentState(exec);
> > to ScheduledAction::create.
> > 
> > r- for this
> 
> On second thought, we cannot just put JSMainThreadExecState currentState(exec); into ScheduledAction::create as it wouldn't work for Workers. They also can call setTimeout/Interval which would lead to modification of JSMainThreadExecState::s_mainThreadState from wrong thread.

Ok.

I don't know nearly enough about the inner workings of JSC to determine what I ought to do here. If you have a concrete suggestion, I'm more than happy to implement it. :)

[Mike West]

Created attachment 162738 [details]
Rebased onto ToT.

[Yury Semikhatsky]

(In reply to comment #48)
> (In reply to comment #47)
> > (In reply to comment #46)
> > > (From update of attachment 161022 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=161022&action=review
> > > 
> > > > LayoutTests/http/tests/inspector-enabled/contentSecurityPolicy-blocks-setInterval-expected.txt:7
> > > > + [native code]:1
> > > 
> > > As Vsevolod pointed out this is a regression as now we don't have exact location for JSC. I guess that JSMainThreadExecState::currentState returns 0 in this case because it doesn't use JSMainThreadExecState::call. This can be fixed by adding 
> > > JSMainThreadExecState currentState(exec);
> > > to ScheduledAction::create.
> > > 
> > > r- for this
> > 
> > On second thought, we cannot just put JSMainThreadExecState currentState(exec); into ScheduledAction::create as it wouldn't work for Workers. They also can call setTimeout/Interval which would lead to modification of JSMainThreadExecState::s_mainThreadState from wrong thread.
> 
> Ok.
> 
> I don't know nearly enough about the inner workings of JSC to determine what I ought to do here. If you have a concrete suggestion, I'm more than happy to implement it. :)

I see the following options: 
1) Capture stack trace before the CSP check. The problem is that we spend resources collecting stack trace even if it is not needed(presumably common case).
2) Replace JSMainThreadExecState with JSCurrentThreadExecState which would keep current exec state pointer in a thread local variable. This might cause some performance penalty.
3) Introduce ScriptCallStackBuilder that would be allocated on the C++ call stack and in case of JSC keep a pointer to the current exec state. It can later call createScriptCallStackForInspector(exec) with the saved ExecState. In case of V8 it doesn't need to store anything. Reference to the builder could be passed down to CSPDirectiveList::checkEvalAndReportViolation.

[Mike West]

Created attachment 165756 [details]
Patch

[Mike West]

Hi Yury (and Vsevolod (and Pavel))!

I've been avoiding this ticket for a few weeks because I didn't want to deal with the threading issue. I took another look at it this morning, and realized that we only have the threading problem because I'm trying to be clever and elegant about not passing any state into ContentSecurityPolicy. If I give up on that, and pass in the ExecState* we already have inside JSC::ScheduledAction::create, then I think we can avoid the issue entirely.

Can you take a quick look at this approach? In particular, it involves adding a shim to V8's ScriptCallStackFactory to match the JSC interface, and bringing a bit more context into ContentSecurityPolicy::logToConsole.

If you're alright with the approach, I'll rebaseline the tests and see what the bots think.

Thanks!

[Build Bot]

Comment on attachment 165756 [details]
Patch

Attachment 165756 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14032412

New failing tests:
http/tests/inspector/csp-inline-warning-contains-stacktrace.html
http/tests/inspector/csp-injected-content-warning-contains-stacktrace.html

[Mike West]

Created attachment 165847 [details]
Patch

[Adam Barth]

Comment on attachment 165847 [details]
Patch

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

This is fine, but we don't what to have JSC types in WebCore headers.  That's the problem that ScriptState.h is supposed to solve for us.

> Source/WebCore/page/ContentSecurityPolicy.h:44
> +#if USE(JSC)
> +namespace JSC {
> +class ExecState;
> +}
> +#endif

This shouldn't be needed.

> Source/WebCore/page/ContentSecurityPolicy.h:57
> +#if USE(JSC)
> +typedef JSC::ExecState ScriptState;
> +#else
> +class ScriptState;
> +#endif

Why not just include ScriptState.h ?

[Mike West]

(In reply to comment #55)
> (From update of attachment 165847 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=165847&action=review
> 
> This is fine, but we don't what to have JSC types in WebCore headers.  That's the problem that ScriptState.h is supposed to solve for us.

This solution was copy/pasted from InspectorInstrumentation.h. I'll update this patch, and I'm happy to change that other file to use ScriptState.h in a separate patch, if you like.

[Mike West]

Created attachment 165935 [details]
Patch

[Mike West]

(In reply to comment #56)
> This solution was copy/pasted from InspectorInstrumentation.h. I'll update this patch, and I'm happy to change that other file to use ScriptState.h in a separate patch, if you like.

I've updated the patch on this bug, and uploaded https://bugs.webkit.org/show_bug.cgi?id=97759 to address InspectorInstrumentation.h.

WDYT?

[Mike West]

Comment on attachment 165935 [details]
Patch

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

> Source/WebCore/page/ContentSecurityPolicy.cpp:1630
> +    if (InspectorInstrumentation::hasFrontends()) {

I gather from https://bugs.webkit.org/show_bug.cgi?id=96730 (thanks Adam!) that `hasFrontends` isn't what I should be checking here. I'm actually not sure if I should be checking for frontends at all. I think we probably want the stacktrace to be generated even if the Inspector isn't open...

Pavel, et al? WDYT?

[Yury Semikhatsky]

Comment on attachment 165935 [details]
Patch

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

>> Source/WebCore/page/ContentSecurityPolicy.cpp:1630
>> +    if (InspectorInstrumentation::hasFrontends()) {
> 
> I gather from https://bugs.webkit.org/show_bug.cgi?id=96730 (thanks Adam!) that `hasFrontends` isn't what I should be checking here. I'm actually not sure if I should be checking for frontends at all. I think we probably want the stacktrace to be generated even if the Inspector isn't open...
> 
> Pavel, et al? WDYT?

Having the stack traces generated even when there is no front-end is better from the web developer perspective but what if there is some function call violating CSP in a loop? This is probably a rare case and is likely an error anyway, but if we don't have precise information about that I'd rather put some check to be on the safe side.

InspectorInstrumentation::hasFrontends check could be replaced with InspectorInstrumentation::consoleAgentEnabled() as the stack trace will be saved in a console message.

[Mike West]

(In reply to comment #60)
> Having the stack traces generated even when there is no front-end is better from the web developer perspective but what if there is some function call violating CSP in a loop? This is probably a rare case and is likely an error anyway, but if we don't have precise information about that I'd rather put some check to be on the safe side.

Someone could maliciously generate code that generated errors in a loop, but it would be pretty obviously an error. Still...
 
> InspectorInstrumentation::hasFrontends check could be replaced with InspectorInstrumentation::consoleAgentEnabled() as the stack trace will be saved in a console message.

I'll throw this in. Thanks for your help!

[Mike West]

Created attachment 166048 [details]
Patch

[Build Bot]

Comment on attachment 166048 [details]
Patch

Attachment 166048 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14034932

New failing tests:
http/tests/inspector-enabled/contentSecurityPolicy-blocks-setInterval.html
http/tests/inspector-enabled/contentSecurityPolicy-blocks-setTimeout.html

[WebKit Review Bot]

Comment on attachment 166048 [details]
Patch

Attachment 166048 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14036931

New failing tests:
http/tests/inspector-enabled/contentSecurityPolicy-blocks-setInterval.html
http/tests/inspector-enabled/contentSecurityPolicy-blocks-setTimeout.html

[Mike West]

Created attachment 166068 [details]
Patch

[Mike West]

(In reply to comment #65)
> Created an attachment (id=166068) [details]
> Patch

Uploading a patch without a consoleAgentEnabled check. I'd really like to get a stack trace even if the console isn't open; asking a developer to reload the page after opening devtools is something I'd like to avoid if possible.

I'll ping you tomorrow to chat about it, Yury. If I can't change your mind, then maybe you can help me fiddle with the inspector-enabled tests to make them work with that check. :)

[Pavel Feldman]

> Uploading a patch without a consoleAgentEnabled check. I'd really like to get a stack trace even if the console isn't open; asking a developer to reload the page after opening devtools is something I'd like to avoid if possible.

At this moment, we only capture traces for console messages when console is enabled. Otherwise it can affect page runtime and footprint. I'd recommend doing it consistently.

[Mike West]

(In reply to comment #67)
> > Uploading a patch without a consoleAgentEnabled check. I'd really like to get a stack trace even if the console isn't open; asking a developer to reload the page after opening devtools is something I'd like to avoid if possible.
> 
> At this moment, we only capture traces for console messages when console is enabled. Otherwise it can affect page runtime and footprint. I'd recommend doing it consistently.

Good timing, I was just about to hop on IRC to ask about this. :)

Ok. I'll throw the check back in, and rewrite the http/tests/inspector-enabled/contentSecurityPolicy-* tests to ensure that the console is open. Thanks for the feedback.

[Mike West]

Created attachment 166201 [details]
Patch

[Build Bot]

Comment on attachment 166201 [details]
Patch

Attachment 166201 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14060382

New failing tests:
http/tests/inspector/csp-setInterval-warning-contains-stacktrace.html
http/tests/workers/terminate-during-sync-operation.html
http/tests/inspector/csp-setTimeout-warning-contains-stacktrace.html

[Mike West]

Created attachment 166278 [details]
Patch

[Mike West]

96730 got rolled out, so my patch failed. :(

Inspector folks, do you expect to reland that soonish? Or should I rebase this onto ToT without that patch?

[Vsevolod Vlasov]

(In reply to comment #72)
> 96730 got rolled out, so my patch failed. :(
> 
> Inspector folks, do you expect to reland that soonish? Or should I rebase this onto ToT without that patch?

I think Pavel will quickly fix this crash (seems like nothing special there), so I would wait.

[Mike West]

(In reply to comment #73)
> (In reply to comment #72)
> > 96730 got rolled out, so my patch failed. :(
> > 
> > Inspector folks, do you expect to reland that soonish? Or should I rebase this onto ToT without that patch?
> 
> I think Pavel will quickly fix this crash (seems like nothing special there), so I would wait.

Easy. Thanks! :)

[Mike West]

Created attachment 166440 [details]
Rebased.

[Mike West]

(In reply to comment #75)
> Created an attachment (id=166440) [details]
> Rebased.

Pavel relanded his change, and the bots are happy with mine. I'll wait until tomorrow to make sure the change sticks, and then land this.

[WebKit Review Bot]

Comment on attachment 166440 [details]
Rebased.

Clearing flags on attachment: 166440

Committed r130150: <http://trac.webkit.org/changeset/130150>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.