Add ExceptionHandler to ModuleSystem, remove heap allocated v8::TryCatch.

Add ExceptionHandler to ModuleSystem, remove heap allocated v8::TryCatch.


BUG=152389
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=170898

[koz (OOO until 15th September), 2012-11-08 23:52:30]

This fixes a problem where v8::TryCatch was being allocated on the stack (see
bug for details). In fixing that I realised that ModuleSystemTest's TryCatch
wasn't doing anything anyway, because ModuleSystem itself has TryCatches that
supercede it.

[haraken, 2012-11-12 09:02:16]

Thanks koz for the patch!

aa: This bug has been blocking a V8 bug, which has been blocking a bunch of
WebKit bugs. Would you review the patch?:)


On 2012/11/08 23:52:30, koz wrote:
> This fixes a problem where v8::TryCatch was being allocated on the stack (see
> bug for details). In fixing that I realised that ModuleSystemTest's TryCatch
> wasn't doing anything anyway, because ModuleSystem itself has TryCatches that
> supercede it.

[koz (OOO until 15th September), 2012-11-13 23:15:15]

On 2012/11/12 09:02:16, haraken1 wrote:
> Thanks koz for the patch!
> 
> aa: This bug has been blocking a V8 bug, which has been blocking a bunch of
> WebKit bugs. Would you review the patch?:)
> 
> 
> On 2012/11/08 23:52:30, koz wrote:
> > This fixes a problem where v8::TryCatch was being allocated on the stack
(see
> > bug for details). In fixing that I realised that ModuleSystemTest's TryCatch
> > wasn't doing anything anyway, because ModuleSystem itself has TryCatches
that
> > supercede it.

+kalman: Mind reviewing this patch?

[not at google - send to devlin, 2012-11-14 00:00:11]

lgtm

http://codereview.chromium.org/11312157/diff/1/chrome/renderer/extensions/mod...
File chrome/renderer/extensions/module_system.h (right):

http://codereview.chromium.org/11312157/diff/1/chrome/renderer/extensions/mod...
chrome/renderer/extensions/module_system.h:110: void HandleException(const
v8::TryCatch& try_catch);
why does this need to be public?

[koz (OOO until 15th September), 2012-11-14 06:14:29]

Thanks!

http://codereview.chromium.org/11312157/diff/1/chrome/renderer/extensions/mod...
File chrome/renderer/extensions/module_system.h (right):

http://codereview.chromium.org/11312157/diff/1/chrome/renderer/extensions/mod...
chrome/renderer/extensions/module_system.h:110: void HandleException(const
v8::TryCatch& try_catch);
On 2012/11/14 00:00:11, kalman wrote:
> why does this need to be public?

Oops, it doesn't. Fixed.

[commit-bot: I haz the power, 2012-11-14 06:15:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/koz@chromium.org/11312157/6001

[commit-bot: I haz the power, 2012-11-14 06:15:26]

Presubmit check for 11312157-6001 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome/test

[koz (OOO until 15th September), 2012-11-14 06:26:23]

On 2012/11/14 06:15:26, I haz the power (commit-bot) wrote:
> Presubmit check for 11312157-6001 failed and returned exit status 1.
> 
> 
> Running presubmit commit checks ...
> 
> ** Presubmit ERRORS **
> Missing LGTM from an OWNER for files in these directories:
>     chrome/test

+sky: Could you rubber stamp for chrome/test OWNERS?

[koz (OOO until 15th September), 2012-11-28 05:54:40]

On 2012/11/14 06:26:23, koz wrote:
> On 2012/11/14 06:15:26, I haz the power (commit-bot) wrote:
> > Presubmit check for 11312157-6001 failed and returned exit status 1.
> > 
> > 
> > Running presubmit commit checks ...
> > 
> > ** Presubmit ERRORS **
> > Missing LGTM from an OWNER for files in these directories:
> >     chrome/test
> 
> +sky: Could you rubber stamp for chrome/test OWNERS?

Friendly ping.

[sky, 2012-11-28 15:32:41]

I'm not familiar with this particular code. Maybe Pawel is (added a reviewer).

[Paweł Hajdan Jr., 2012-11-28 17:56:44]

LGTM

I'm not that familiar either, but sufficiently for an OWNERS review, and it
seems kalman was reviewing that code before and he also is now, so that should
count as a "main" review.

Note: if no non-OWNER reviewer has reviewed the test files, please make sure
that you get a non-OWNER review of that. You have my stamp anyway.

[koz (OOO until 15th September), 2012-11-28 23:17:40]

On 2012/11/28 17:56:44, Paweł Hajdan Jr. wrote:
> LGTM
> 
> I'm not that familiar either, but sufficiently for an OWNERS review, and it
> seems kalman was reviewing that code before and he also is now, so that should
> count as a "main" review.
> 
> Note: if no non-OWNER reviewer has reviewed the test files, please make sure
> that you get a non-OWNER review of that. You have my stamp anyway.

[+mstarzinger]

Michael, mind reviewing?

[Michael Starzinger, 2012-11-29 09:15:13]

Drive-by LGTM.

[commit-bot: I haz the power, 2012-11-29 22:31:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/koz@chromium.org/11312157/6001

[commit-bot: I haz the power, 2012-11-29 22:57:11]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_clang.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_clan...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-11-30 05:53:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/koz@chromium.org/11312157/16002

[commit-bot: I haz the power, 2012-11-30 05:53:49]

Presubmit check for 11312157-16002 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome

Presubmit checks took 1.3s to calculate.

Was the presubmit check useful? Please send feedback & hate mail to
maruel@chromium.org!

[koz (OOO until 15th September), 2012-11-30 06:05:34]

[+sky]

Scott, mind doing an OWNERS review for chrome/?

[sky, 2012-11-30 17:28:53]

What parts of this do you need me to review?

[koz (OOO until 15th September), 2012-11-30 18:20:18]

On 2012/11/30 17:28:53, sky wrote:
> What parts of this do you need me to review?

I think the script is complaining about the files in chrome/renderer.

[sky, 2012-12-03 14:48:36]

resource_bundle_map LGTM

[commit-bot: I haz the power, 2012-12-03 22:38:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/koz@chromium.org/11312157/16002

[commit-bot: I haz the power, 2012-12-04 00:09:17]

Retried try job too often on linux_rel for step(s) unit_tests

[commit-bot: I haz the power, 2012-12-04 03:26:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/koz@chromium.org/11312157/20004

[commit-bot: I haz the power, 2012-12-04 07:23:24]

Change committed as 170898