Adding checks for V8 functions returning Maybe<bool>

Adding checks for V8 functions returning Maybe<bool>

While working on an V8 issue [1], involving adding V8_WARN_UNUSED_RESULT
to functions that were missing it, we discovered that a few calls to
these functions were not using the returned value.

This commit allows compliation to succeed, but in most cases only adds a
comment that something should be done. I'm opening this CL to get
feedback on what the appropriate actions should be.

To try this out I used a .gclient with a custom_deps like this:
u'custom_deps': {
  "src/v8":
"git@github.com:danbev/v8.git@064718a8921608eaf9b5eadbb7d734ec04068a87"
},

[1] https://codereview.chromium.org/2135973002/

BUG=

[danbev, 2016-07-14 04:54:21]

daniel.bevenius@gmail.com changed reviewers:
+ jochen@chromium.org

[jochen (gone - plz use gerrit), 2016-07-14 15:03:23]

jochen@chromium.org changed reviewers:
+ haraken@chromium.org

[jochen (gone - plz use gerrit), 2016-07-14 15:03:23]

+haraken for blink bindings changes

https://codereview.chromium.org/2144093002/diff/1/extensions/renderer/module_...
File extensions/renderer/module_system.cc (right):

https://codereview.chromium.org/2144093002/diff/1/extensions/renderer/module_...
extensions/renderer/module_system.cc:460: if (object->Delete(context,
property).IsNothing()) {
I'd just crash / add NOTREACHED()

[haraken, 2016-07-15 02:38:41]

https://codereview.chromium.org/2144093002/diff/20001/extensions/renderer/mod...
File extensions/renderer/module_system.cc (right):

https://codereview.chromium.org/2144093002/diff/20001/extensions/renderer/mod...
extensions/renderer/module_system.cc:584: // TODO: What action, if any, should
be taken here?

I think we should just crash.

https://codereview.chromium.org/2144093002/diff/20001/extensions/renderer/mod...
extensions/renderer/module_system.cc:760: // TODO: What action, if any, should
be taken here?

Ditto.

https://codereview.chromium.org/2144093002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/ScriptPromise.cpp (right):

https://codereview.chromium.org/2144093002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/ScriptPromise.cpp:186: if
(m_resolver.v8Value().As<v8::Promise::Resolver>()->Resolve(

Can you use v8Call macros in V8BindingMacros.h?

You can use v8CallOrCrash to just crash when V8 returns an empty Maybe handle.
The same comment for other files in bindings/.

[danbev, 2016-07-15 11:07:55]

On 2016/07/15 02:38:41, haraken wrote:
>
https://codereview.chromium.org/2144093002/diff/20001/extensions/renderer/mod...
> File extensions/renderer/module_system.cc (right):
> 
>
https://codereview.chromium.org/2144093002/diff/20001/extensions/renderer/mod...
> extensions/renderer/module_system.cc:584: // TODO: What action, if any, should
> be taken here?
> 
> I think we should just crash.
> 
>
https://codereview.chromium.org/2144093002/diff/20001/extensions/renderer/mod...
> extensions/renderer/module_system.cc:760: // TODO: What action, if any, should
> be taken here?
> 
> Ditto.
> 
I just wanted to double check one thing regarding module_system.cc. I used
NOTREACHED() in the previous commit,  but noticed that there are calls that
handle a Maybe<bool> result differently, for example
https://cs.chromium.org/chromium/src/extensions/renderer/module_system.cc?q=S...
Should I use that pattern as well perhaps?

[jochen (gone - plz use gerrit), 2016-07-18 11:29:30]

On 2016/07/15 at 11:07:55, daniel.bevenius wrote:
> On 2016/07/15 02:38:41, haraken wrote:
> >
https://codereview.chromium.org/2144093002/diff/20001/extensions/renderer/mod...
> > File extensions/renderer/module_system.cc (right):
> > 
> >
https://codereview.chromium.org/2144093002/diff/20001/extensions/renderer/mod...
> > extensions/renderer/module_system.cc:584: // TODO: What action, if any,
should
> > be taken here?
> > 
> > I think we should just crash.
> > 
> >
https://codereview.chromium.org/2144093002/diff/20001/extensions/renderer/mod...
> > extensions/renderer/module_system.cc:760: // TODO: What action, if any,
should
> > be taken here?
> > 
> > Ditto.
> > 
> I just wanted to double check one thing regarding module_system.cc. I used
NOTREACHED() in the previous commit,  but noticed that there are calls that
handle a Maybe<bool> result differently, for example
https://cs.chromium.org/chromium/src/extensions/renderer/module_system.cc?q=S...
> Should I use that pattern as well perhaps?

that's what I meant with my initial comment that we should just crash if the
call fails

[danbev, 2016-07-18 12:14:56]

On 2016/07/18 11:29:30, jochen wrote:
> On 2016/07/15 at 11:07:55, daniel.bevenius wrote:
> > On 2016/07/15 02:38:41, haraken wrote:
> > >
>
https://codereview.chromium.org/2144093002/diff/20001/extensions/renderer/mod...
> > > File extensions/renderer/module_system.cc (right):
> > > 
> > >
>
https://codereview.chromium.org/2144093002/diff/20001/extensions/renderer/mod...
> > > extensions/renderer/module_system.cc:584: // TODO: What action, if any,
> should
> > > be taken here?
> > > 
> > > I think we should just crash.
> > > 
> > >
>
https://codereview.chromium.org/2144093002/diff/20001/extensions/renderer/mod...
> > > extensions/renderer/module_system.cc:760: // TODO: What action, if any,
> should
> > > be taken here?
> > > 
> > > Ditto.
> > > 
> > I just wanted to double check one thing regarding module_system.cc. I used
> NOTREACHED() in the previous commit,  but noticed that there are calls that
> handle a Maybe<bool> result differently, for example
>
https://cs.chromium.org/chromium/src/extensions/renderer/module_system.cc?q=S...
> > Should I use that pattern as well perhaps?
> 
> that's what I meant with my initial comment that we should just crash if the
> call fails

Ah I see, sorry I misunderstood. I'll add a commit for this.

[danbev, 2016-07-18 14:29:58]

Updating extensions/renderer/module_system.cc with this commit.

[jochen (gone - plz use gerrit), 2016-07-19 12:18:40]

extensions/ look good

[haraken, 2016-07-19 19:57:17]

WebKit LGTM

[danbev, 2016-07-20 05:13:30]

The CQ bit was checked by daniel.bevenius@gmail.com

[commit-bot: I haz the power, 2016-07-20 05:13:51]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-20 05:22:39]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-20 05:22:40]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[danbev, 2016-07-20 16:04:01]

On 2016/07/20 05:22:40, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

Sorry, I think I hit check the commit box too early :( I believe that this
cannot be committed before the accompanying V8 tasks is committed.

[jochen (gone - plz use gerrit), 2016-07-21 08:28:05]

also, you still need my approval :)

lgtm

[danbev, 2016-07-21 16:52:10]

On 2016/07/21 08:28:05, jochen wrote:
> also, you still need my approval :)
> 
> lgtm

Sorry about that, won't happen again. 

I've rebased and update the V8 https://codereview.chromium.org/2135973002 CL,
and run through the compilation here in chromium using those changes.

[danbev, 2017-01-25 06:33:57]

I've rebased this with upstream master and reapplied the changes as it has been
so long since I did the last update (sorry about that, it was not lack of
interest). 

My .gclient for running this looks like this:
$ cat ../.gclient
solutions = [{u'managed': False, u'name': u'src', u'url':
u'https://chromium.googlesource.com/chromium/src.git',
u'custom_deps': {
  "src/v8":
"git@github.com:danbev/v8.git@92f8e9d2b3156eeda2a6c8829005317e99f0fed8"
}, u'deps_file': u'.DEPS.git', u'safesync_url': u''}]

The commit is for an V8 issue:
https://codereview.chromium.org/2135973002/

[jochen (gone - plz use gerrit), 2017-01-25 12:20:17]

The CQ bit was checked by jochen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-25 12:20:36]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-25 13:40:39]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-25 13:40:40]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[danbev, 2017-03-19 10:25:32]

Rebased with master and tested (chrome/test:unit_tests) using .gclient:

solutions = [{u'managed': False, u'name': u'src', u'url':
u'https://chromium.googlesource.com/chromium/src.git',
u'custom_deps': {
  "src/v8":
"git@github.com:danbev/v8.git@9a9333cdf7933326936929dd530de98e72168b24"
}, u'deps_file': u'.DEPS.git', u'safesync_url': u''}]

[jochen (gone - plz use gerrit), 2017-03-20 07:12:04]

The CQ bit was checked by jochen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-20 07:12:19]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jochen (gone - plz use gerrit), 2017-03-20 07:12:50]

I triggered a dry run. CL still lgtm, so feel free to hit the commit button if
the bots are green

[commit-bot: I haz the power, 2017-03-20 08:52:36]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-20 08:52:37]

Dry run: This issue passed the CQ dry run.

[danbev, 2017-03-20 08:54:57]

The CQ bit was checked by daniel.bevenius@gmail.com

[danbev, 2017-03-20 08:54:57]

The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org
Link to the patchset: https://codereview.chromium.org/2144093002/#ps100001
(title: "Adding checks for V8 functions returning Maybe<bool>")

[commit-bot: I haz the power, 2017-03-20 08:55:06]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-20 09:01:01]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1490000097042580,
"parent_rev": "f4292e56c05fc2ad1710c4b691923a40a4d115f3", "commit_rev":
"33ac37fdb8da49817a14b12205afa06d928476cb"}

[commit-bot: I haz the power, 2017-03-20 09:06:20]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-20 09:06:22]

Prior attempt to commit was detected, but we were not able to check whether the
issue was successfully committed. Please check Git history manually and re-check
CQ or close this issue as needed.

[jochen (gone - plz use gerrit), 2017-03-20 09:07:17]

The CQ bit was checked by jochen@chromium.org

[commit-bot: I haz the power, 2017-03-20 09:07:32]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-20 09:15:27]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-20 09:15:28]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[jochen (gone - plz use gerrit), 2017-03-22 09:56:33]

ok, this landed as
https://chromium.googlesource.com/chromium/src/+/33ac37fdb8da49817a14b12205af...
... closing

[danbev, 2017-03-22 10:44:30]

On 2017/03/22 09:56:33, jochen wrote:
> ok, this landed as
>
https://chromium.googlesource.com/chromium/src/+/33ac37fdb8da49817a14b12205af...
> ... closing

Thanks for all your help and patience on this!