Bug 94332

Hey Ulan! We talked about this change a few weeks back; https://bugs.webkit.org/show_bug.cgi?id=94331 is a patch to change the message in JSC. I'd like to get your help changing the message in V8. Could you take a look at the JSC patch and help me evaluate how to make it work in V8? Thanks!

Created attachment 159662 [details] Patch

Hi Mike, I uploaded a draft patch that depends on your patch and on this V8 CL: https://chromiumcodereview.appspot.com/10837358/ At the moment it passes tests, but there is one call site in WebKit where we need to pass the error message: WebKit/Source/WebCore/bindings/v8/ScriptState.cpp : setEvalEnabled(ScriptState* scriptState, bool enabled) It is used from inspector. Do you know how we can pass error message there?

Thanks Ulan. Let's let the JSC change land first (https://bugs.webkit.org/show_bug.cgi?id=94331); the patch to wire things up to the new V8 patch you've put together should then be quite small. It looks like fixing the Web Inspector bit should be pretty straightforward as well. I'll just add some code to the bindings to expose the ErrorMessageForCodeGenerationFromStrings method that you've added, and then we should be able to save/rewrite that rather than just toggling the boolean. Thank you!

Hey Ulan! The JSC patch just landed last night. *phew* :) I'll rebase this patch on top of the current trunk. Would you mind taking another look at landing https://codereview.chromium.org/10837358/ in the meantime? I think we'll need that in place before we can land the WebKit side.

Created attachment 164279 [details] Patch

(In reply to comment #6) > Created an attachment (id=164279) [details] > Patch I'll rebaseline the tests once the V8 portion of this change lands and is rolled into WebKit. It doesn't yet take care of ScriptState.cpp, as I think we'll need to chat about what the V8 API needs to look like. I'll poke you on Monday, Uan. :) Thanks!

Comment on attachment 164279 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=164279&action=review Dr. Barth is your man. > Source/WebCore/bindings/v8/V8DOMWindowShell.cpp:373 > + context->AllowCodeGenerationFromStrings(m_frame->document()->contentSecurityPolicy()->allowEval(0, ContentSecurityPolicy::SuppressReport), v8String(m_frame->document()->contentSecurityPolicy()->evalDisabledErrorMessage())); A ContentSecurityPolicy local would make this line much shorter. :) > Source/WebCore/bindings/v8/WorkerContextExecutionProxy.cpp:89 > + , m_disableEvalPending(String()) I can never remember if we have a nullString() which wer'e supposed to use instead these days.

Comment on attachment 164279 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=164279&action=review >> Source/WebCore/bindings/v8/WorkerContextExecutionProxy.cpp:89 >> + , m_disableEvalPending(String()) > > I can never remember if we have a nullString() which wer'e supposed to use instead these days. You can actually skip this line. The compiler calls the default constructor for you. nullAtom is only needed when you're returning something by reference.

Comment on attachment 164279 [details] Patch Patch looks fine.

Created attachment 164357 [details] New method.

Hey Ulan, here's an updated version of the patch to match the API changes you've made. It doesn't compile without those changes, so... Let's hope it compiles on your machine. :)

(In reply to comment #12) > Hey Ulan, here's an updated version of the patch to match the API changes you've made. It doesn't compile without those changes, so... Let's hope it compiles on your machine. :) Yep, it compiles and produces the following outputs for eval-blocked-in-about-blank-iframe.html, eval-blocked.html, function-constructor-blocked.html: CONSOLE MESSAGE: line 1: Uncaught EvalError: Refused to evaluate a string as JavaScript because 'unsafe-eval' is not an allowed source of script in the following Content Security Policy directive: "script-src 'unsafe-inline'". CONSOLE MESSAGE: line 12: Uncaught EvalError: Refused to evaluate a string as JavaScript because 'unsafe-eval' is not an allowed source of script in the following Content Security Policy directive: "script-src 'unsafe-inline'". CONSOLE MESSAGE: line 15: Uncaught EvalError: Refused to evaluate a string as JavaScript because 'unsafe-eval' is not an allowed source of script in the following Content Security Policy directive: "script-src 'unsafe-inline'". ONSOLE MESSAGE: line 12: Uncaught EvalError: Refused to evaluate a string as JavaScript because 'unsafe-eval' is not an allowed source of script in the following Content Security Policy directive: "script-src 'unsafe-inline'".

(In reply to comment #13) > Yep, it compiles and produces the following outputs for > eval-blocked-in-about-blank-iframe.html, eval-blocked.html, function-constructor-blocked.html: Looks good. Would you mind landing the V8 side of the patch? Once it gets rolled into the Chromium port, I'll see about landing this side. Thanks, Ulan!

I landed the V8 CL as http://code.google.com/p/v8/source/detail?r=12525 to V8 bleeding_edge. It looks like we won't roll V8 until Chrome branches (V8 trunk is frozen).

(In reply to comment #15) > I landed the V8 CL as http://code.google.com/p/v8/source/detail?r=12525 to V8 bleeding_edge. It looks like we won't roll V8 until Chrome branches (V8 trunk is frozen). Makes sense. So, what timeframe are we looking at? Beginning of next weekish?

> So, what timeframe are we looking at? Beginning of next weekish? Yes. Probably, next week Wed.

Created attachment 164926 [details] Rebaselining.

(In reply to comment #18) > Created an attachment (id=164926) [details] > Rebaselining. I've rolled DEPS locally and rebaselined the tests. I think we should be able to land this directly after the V8 change rolls into WebKit ToT next week.

Created attachment 165266 [details] Patch

Comment on attachment 165266 [details] Patch Dropping the r?, throwing to the bots. If the bots are happy, I'll carry Adam's r+ over and ask for CQ?. :)

V8 roll that this patch depends on is going to be reverted because of high crash rate :(

(In reply to comment #22) > V8 roll that this patch depends on is going to be reverted because of high crash rate :( Ah, too bad! Good that I waited until Monday. :)

(In reply to comment #23) > (In reply to comment #22) > > V8 roll that this patch depends on is going to be reverted because of high crash rate :( Looks like the next try at a roll was reverted too. :(

(In reply to comment #24) > (In reply to comment #23) > > (In reply to comment #22) > > > V8 roll that this patch depends on is going to be reverted because of high crash rate :( > > Looks like the next try at a roll was reverted too. :( And the next. I'm putting this on ice until the V8 roll happens and sticks. Probably next week? Bleh. :/

Created attachment 168159 [details] Rebasing onto ToT

Created attachment 169126 [details] Rebased again, for the last time (maybe).

V8 has rolled in the fix I was waiting for, and a fix to that fix. Things work locally; throwing this into the queue. :)

Comment on attachment 169126 [details] Rebased again, for the last time (maybe). Clearing flags on attachment: 169126 Committed r131573: <http://trac.webkit.org/changeset/131573>

All reviewed patches have been landed. Closing bug.