Closed
Bug 1197973
Opened 8 years ago
Closed 7 years ago
Add MOZ_MUST_USE to AutoJSAPI::Init
Categories
(Core :: General, defect)
Core
General
Tracking
()
RESOLVED
FIXED
mozilla50
People
(Reporter: jdm, Assigned: n.nethercote, Mentored)
References
(Blocks 2 open bugs)
Details
(Keywords: coverity, Whiteboard: [lang=c++][CID 1363740])
Attachments
(1 file, 3 obsolete files)
14.55 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Comment 1•8 years ago
|
||
http://mxr.mozilla.org/mozilla-central/source/mfbt/Attributes.h#463 https://dxr.mozilla.org/mozilla-central/source/dom/base/ScriptSettings.h#213
Mentor: josh
Whiteboard: [lang=c++]
Reporter | ||
Comment 3•8 years ago
|
||
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)
Comment 4•8 years ago
|
||
Ok, I will try this. Can you assign this to me, please?
Flags: needinfo?(josh)
Comment 5•8 years ago
|
||
Ok, I will try this. Can you assign this to me, please?
Reporter | ||
Comment 6•8 years ago
|
||
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)
Comment 7•8 years ago
|
||
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)
Reporter | ||
Comment 8•8 years ago
|
||
We can declare it right next to AutoJSAPI. We should change any Init method that returns a boolean value right now.
Flags: needinfo?(josh)
Comment 9•8 years ago
|
||
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)
Reporter | ||
Comment 10•8 years ago
|
||
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)
Comment 11•8 years ago
|
||
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)
Comment 12•8 years ago
|
||
Return true for the void Init instead of false.
Attachment #8684505 -
Attachment is obsolete: true
Reporter | ||
Comment 14•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8ead6461c46a
Reporter | ||
Comment 15•8 years ago
|
||
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)
Reporter | ||
Updated•8 years ago
|
Blocks: use-nodiscard
Reporter | ||
Comment 16•8 years ago
|
||
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
Reporter | ||
Updated•8 years ago
|
Attachment #8684508 -
Attachment is obsolete: true
![]() |
Assignee | |
Comment 17•7 years ago
|
||
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 | |
Updated•7 years ago
|
Assignee: nobody → n.nethercote
Status: NEW → ASSIGNED
![]() |
Assignee | |
Updated•7 years ago
|
Reporter | ||
Comment 18•7 years ago
|
||
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)
![]() |
Assignee | |
Comment 19•7 years ago
|
||
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 20•7 years ago
|
||
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+
![]() |
Assignee | |
Comment 21•7 years ago
|
||
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)
![]() |
Assignee | |
Updated•7 years ago
|
Attachment #8770015 -
Attachment is obsolete: true
![]() |
||
Comment 22•7 years ago
|
||
Comment on attachment 8770761 [details] [diff] [review] Use MOZ_MUST_USE in AutoJSAPI r=me
Attachment #8770761 -
Flags: review?(bzbarsky) → review+
![]() |
Assignee | |
Comment 23•7 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/5102f1b9bccae81f88e8431c817d6ecce5c45529 Bug 1197973 - Use MOZ_MUST_USE in AutoJSAPI. r=bz.
Comment 24•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/5102f1b9bcca
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox50:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in
before you can comment on or make changes to this bug.
Description
•