1197973 - Add MOZ_MUST_USE to AutoJSAPI::Init

Status: RESOLVED FIXED

(Core :: General, defect)

Description

[Josh Matthews [:jdm]]

It is always incorrect not to check the result of AutoJSAPI::Init, therefore we should actually enforce that invariant. There doesn't seem to be a way to enforce that arbitrary method return types should be used, so let's create a new type that can silently convert to a boolean value instead.

Comment 1

[Josh Matthews [:jdm]]

http://mxr.mozilla.org/mozilla-central/source/mfbt/Attributes.h#463
https://dxr.mozilla.org/mozilla-central/source/dom/base/ScriptSettings.h#213

Comment 2

[Ninad Bhat[:ninad101]]

Would this be a good first bug to take?

Comment 3

[Josh Matthews [:jdm]]

The requested change should not be difficult. Addressing the places that currently do not check the return value may be more difficult, but there's only one way to find out :)

Comment 4

[Ninad Bhat[:ninad101]]

Ok, I will try this. Can you assign this to me, please?

Comment 5

[Ninad Bhat[:ninad101]]

Ok, I will try this. Can you assign this to me,  please?

Comment 6

[Josh Matthews [:jdm]]

The policy is to assign bugs to new contributors once a patch has been submitted. Comment 4 is enough to claim this for yourself :)

Comment 7

[Tareq Khandaker]

I have a few questions:

1) Where should the new type be declared and defined?
2) Will the other Init functions need to be changed or is that outside the scope of this particular bug?

Comment 8

[Josh Matthews [:jdm]]

We can declare it right next to AutoJSAPI. We should change any Init method that returns a boolean value right now.

Comment 9

[Tareq Khandaker]

More questions:

1) I called the type "CheckedBoolean". Is this name alright or do you have a more appropriate name for this class.

2) I declared the type like so:

class MOZ_MUST_USE CheckedBoolean {
public:
  CheckedBoolean();
  CheckedBoolean(bool value);
  ~CheckedBoolean();

  explicit operator bool() const;

private:
  bool value;
};

and I changed all the Init methods, including the void one, to return CheckedBooleans. I did not get any errors about not reading the value of this type, so I am a little worried that I might be doing things wrong. Any idea of what might be wrong? I can make a patch and send it up for review if it makes things easier.

Comment 10

[Josh Matthews [:jdm]]

I apologize for the long delay here - I was at a conference last week, and then forgot about this :(
1) A name like MustUseBoolean is clearer to me.
2) That's a curious result. Which operating system are you developing on? Have you tried submitting your patch to the tryserver?

Comment 11

[Tareq Khandaker]

Attachment: bug1197973.patch

I am building on OS X 10.11 . I requested try access (I think you already know since a clone was created).

I just built this patch using ./mach build and it built without errors. Once again, there were no complaints about the value not being read when I think there should be.

Comment 12

[Tareq Khandaker]

Attachment: bug1197973.patch - Return true for the void Init

Return true for the void Init instead of false.

Comment 13

[Tareq Khandaker]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1adbfb3ded4e

Comment 14

[Josh Matthews [:jdm]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8ead6461c46a

Comment 15

[Josh Matthews [:jdm]]

I apologize for going silent here. It looks like the errors for must-use types are generated by a Clang plugin that only runs on the static analysis builds (ie. the S on treeherder), and unfortunately the changes triggered another static analysis error that prevented the expected ones from appearing. I've pushed another run with that fixed, so we should be able to see the expected failures now!

Comment 16

[Josh Matthews [:jdm]]

I just learned about MOZ_MUST_USE, which is exactly what we want for this and will avoid the problem with the explicit boolean conversions that MOZ_MUST_USE_TYPE required.

Comment 17

[Nicholas Nethercote [inactive]]

Attachment: Use MOZ_MUST_USE in AutoJSAPI

I addressed this bug due to a Coverity error report, and I only discovered that
it had already been filed after I had written the patch.

jdm, are you the best person to review? I am not sure about a couple of the
changes, specifically the ones that involved |return;| and |return true;|.

Comment 18

[Josh Matthews [:jdm]]

Comment on attachment 8770015 [details] [diff] [review]
Use MOZ_MUST_USE in AutoJSAPI

Review of attachment 8770015 [details] [diff] [review]:
-----------------------------------------------------------------

I don't think I can review the modifications to the usages, unfortunately. The people who know those parts of the code should do so.

Comment 19

[Nicholas Nethercote [inactive]]

Comment on attachment 8770015 [details] [diff] [review]
Use MOZ_MUST_USE in AutoJSAPI

bz, is this something you can review?

Comment 20

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8770015 [details] [diff] [review]
Use MOZ_MUST_USE in AutoJSAPI

>+  if (!jsapi.Init(&aOriginAttributes.toObject())) {
>+    return NS_ERROR_OUT_OF_MEMORY;

I think NS_ERROR_UNEXPECTED is more appropriate here.  I'm not sure there's actually ever a way that the JSObject* version of Init() can return false...

>+  if (!jsapi.Init(xpc::UnprivilegedJunkScope())) {
>+    return NS_ERROR_OUT_OF_MEMORY;

Same here.

>+++ b/dom/fetch/Fetch.cpp
>+  if (!jsapi.Init(aGlobal)) {
>+    return nullptr;

You probably want aRv.Throw(NS_ERROR_NOT_AVAILABLE); before that return statement.  Here in theory things _could_ in fact fail.

>+  if (!jsapi.Init(DerivedClass()->GetParentObject())) {
>+    return;

Probably best to localPromise->MaybeReject(NS_ERROR_UNEXPECTED); before this early return.

>+++ b/dom/html/nsBrowserElement.cpp
>+  if (!jsapi.Init(wrappedObj->GetJSObject())) {
>+    return nullptr;

aRv.Throw(NS_ERROR_UNEXPECTED) here, both callsites in this file.

>+      if (!jsapi.StealException(&exn)) {
>+        return;

I think we should mPromise->MaybeReject(NS_ERROR_OUT_OF_MEMORY) and continue on instead of returning...

>+                bool dummy = aes.StealException(aException);

Does "unused << aes.StealException(aException);" not work?

r=me

Comment 21

[Nicholas Nethercote [inactive]]

Attachment: Use MOZ_MUST_USE in AutoJSAPI

Thank you for the fast and thorough review.

I've requested re-review because of the number of changes made. In particular,
please check the one in Promise.cpp closely, where I added an |else| branch.

Comment 22

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8770761 [details] [diff] [review]
Use MOZ_MUST_USE in AutoJSAPI

r=me

Comment 23

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5102f1b9bccae81f88e8431c817d6ecce5c45529
Bug 1197973 - Use MOZ_MUST_USE in AutoJSAPI. r=bz.

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5102f1b9bcca