Closed Bug 1197973 Opened 4 years ago Closed 3 years ago

Add MOZ_MUST_USE to AutoJSAPI::Init

Categories

(Core :: General, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox43 --- affected
firefox50 --- fixed

People

(Reporter: jdm, Assigned: njn, Mentored)

References

(Blocks 2 open bugs)

Details

(Keywords: coverity, Whiteboard: [lang=c++][CID 1363740])

Attachments

(1 file, 3 obsolete files)

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.
Would this be a good first bug to take?
Flags: needinfo?(josh)
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 :)
Flags: needinfo?(josh)
Ok, I will try this. Can you assign this to me, please?
Flags: needinfo?(josh)
Ok, I will try this. Can you assign this to me,  please?
The policy is to assign bugs to new contributors once a patch has been submitted. Comment 4 is enough to claim this for yourself :)
Flags: needinfo?(josh)
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?
Flags: needinfo?(josh)
We can declare it right next to AutoJSAPI. We should change any Init method that returns a boolean value right now.
Flags: needinfo?(josh)
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.
Flags: needinfo?(josh)
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?
Flags: needinfo?(josh)
Attached patch bug1197973.patch (obsolete) — Splinter Review
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.
Flags: needinfo?(josh)
Return true for the void Init instead of false.
Attachment #8684505 - Attachment is obsolete: true
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!
Flags: needinfo?(josh)
Blocks: MOZ_MUST_USE
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.
Summary: Make AutoJSAPI::Init return a MOZ_MUST_USE type with implicit boolean conversion → Add MOZ_MUST_USE to AutoJSAPI::Init
Attachment #8684508 - Attachment is obsolete: true
Attached patch Use MOZ_MUST_USE in AutoJSAPI (obsolete) — Splinter Review
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;|.
Attachment #8770015 - Flags: review?(josh)
Assignee: nobody → n.nethercote
Status: NEW → ASSIGNED
Keywords: coverity
Whiteboard: [lang=c++] → [lang=c++][CID 1363740]
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.
Attachment #8770015 - Flags: review?(josh)
Comment on attachment 8770015 [details] [diff] [review]
Use MOZ_MUST_USE in AutoJSAPI

bz, is this something you can review?
Attachment #8770015 - Flags: review?(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
Attachment #8770015 - Flags: review?(bzbarsky) → review+
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.
Attachment #8770761 - Flags: review?(bzbarsky)
Attachment #8770015 - Attachment is obsolete: true
Comment on attachment 8770761 [details] [diff] [review]
Use MOZ_MUST_USE in AutoJSAPI

r=me
Attachment #8770761 - Flags: review?(bzbarsky) → review+
https://hg.mozilla.org/mozilla-central/rev/5102f1b9bcca
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in before you can comment on or make changes to this bug.