Closed Bug 1095728 Opened 5 years ago Closed 5 years ago

mark ToJSValue with MOZ_WARN_UNUSED_RESULT

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla37
Tracking Status
firefox35 + wontfix
firefox36 + wontfix
firefox37 --- fixed

People

(Reporter: tromey, Assigned: tromey)

References

(Depends on 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

I recently found some code using ToJSValue and not checking the
return result.  I think this was a latent bug -- if the call
failed for some reason the method would have returned NS_OK
even though something failed.

So I think it may make sense to mark this function with MOZ_WARN_UNUSED_RESULT.
This patch marks most overloads of ToJSValue with
MOZ_WARN_UNUSED_RESULT.  It does not mark a few which always return
true, on the theory that it is reasonable for users to assume this,
and that in any case if the implementation is changed the warning can
be added at that time.

This can't be applied as-is because it reveals the latent bug
mentioned in the original report:

In file included from /home/tromey/firefox-git/gecko-dev/obj-x86_64-unknown-linux-gnu/docshell/base/Unified_cpp_docshell_base0.cpp:56:0:
/home/tromey/firefox-git/gecko-dev/docshell/base/nsDocShell.cpp: In member function ‘virtual nsresult nsDocShell::PopProfileTimelineMarkers(JSContext*, JS::MutableHandle<JS::Value>)’:
/home/tromey/firefox-git/gecko-dev/docshell/base/nsDocShell.cpp:2944:66: error: ignoring return value of ‘bool mozilla::dom::ToJSValue(JSContext*, const nsTArray<E>&, JS::MutableHandle<JS::Value>) [with T = mozilla::dom::ProfileTimelineMarker]’, declared with attribute warn_unused_result [-Werror=unused-result]
   ToJSValue(aCx, profileTimelineMarkers, aProfileTimelineMarkers);
                                                                  ^
cc1plus: all warnings being treated as errors


I didn't fix this directly because I'm working in this area for
bug 1069661.
[Tracking Requested - why for this release]:

[Tracking Requested - why for this release]:

Ignoring the return value of ToJSValue is a major nono.  People should not be doing that, ever.

Is the plan to fix this after fixing bug 1069661, then?

We need to backport at last the docshell fix to 35 as well.
Blocks: 1050376
Flags: needinfo?(ttromey)
(Mumbling about bug 1023295. )
We should fix this asap, even if bug 1023295 doesn't get fixed any time soon.
Depends on: 1023295
(In reply to Boris Zbarsky [:bz] from comment #2)

> Ignoring the return value of ToJSValue is a major nono.  People should not
> be doing that, ever.
> 
> Is the plan to fix this after fixing bug 1069661, then?

My plan was just to fix bug 1069661, since the fix most likely involves
rewriting this code.

> We need to backport at last the docshell fix to 35 as well.

I'm happy write a patch to fix the bug more directly.
Flags: needinfo?(ttromey)
Depends on: 1096329
Assignee: nobody → ttromey
I was preparing this to send to try, so I rebased and it found another bug:

/home/tromey/firefox-git/gecko-dev/dom/workers/ServiceWorkerManager.cpp: In member function ‘void mozilla::dom::workers::UpdatePromise::RejectAllPromises(const mozilla::dom::ErrorEventInit&)’:
/home/tromey/firefox-git/gecko-dev/dom/workers/ServiceWorkerManager.cpp:154:50: error: ignoring return value of ‘bool mozilla::dom::ToJSValue(JSContext*, const nsAString_internal&, JS::MutableHandle<JS::Value>)’, declared with attribute warn_unused_result [-Werror=unused-result]
       ToJSValue(cx, aErrorDesc.mFilename, &fnval);
                                                  ^
/home/tromey/firefox-git/gecko-dev/dom/workers/ServiceWorkerManager.cpp:158:50: error: ignoring return value of ‘bool mozilla::dom::ToJSValue(JSContext*, const nsAString_internal&, JS::MutableHandle<JS::Value>)’, declared with attribute warn_unused_result [-Werror=unused-result]
       ToJSValue(cx, aErrorDesc.mMessage, &msgval);
                                                  ^
Depends on: 1101819
Bug 1101819 is fixed on the maple branch, so I think the patch in
this bug has to wait until that is merged.  I don't know if there is
a way to mark this dependency in bugzilla.
Rebased.  I think this is ready now that the patch for bug 1101819 has
gone in.
Attachment #8519187 - Attachment is obsolete: true
Attachment #8533995 - Flags: review?(bzbarsky)
Comment on attachment 8533995 [details] [diff] [review]
mark ToJSValue with MOZ_WARN_UNUSED_RESULT

r=me
Attachment #8533995 - Flags: review?(bzbarsky) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/e25fb62cf2fd
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
Please nominate for beta/aurora uplift before Mon Dec 22 (for Beta, at least).
Flags: needinfo?(ttromey)
I don't think this one needs to be uplifted.
It is just a preventative measure against future issues like bug 1096329.
Flags: needinfo?(ttromey)
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.