1095728 - mark ToJSValue with MOZ_WARN_UNUSED_RESULT

Status: RESOLVED FIXED

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

Description

[Tom Tromey :tromey]

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.

Comment 1

[Tom Tromey :tromey]

Attachment: mark ToJSValue 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.

Comment 2

[Boris Zbarsky [:bzbarsky]]

[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.

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

(Mumbling about bug 1023295. )

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

We should fix this asap, even if bug 1023295 doesn't get fixed any time soon.

Comment 5

[Tom Tromey :tromey]

(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.

Comment 6

[Tom Tromey :tromey]

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);
                                                  ^

Comment 7

[Tom Tromey :tromey]

https://tbpl.mozilla.org/?tree=Try&rev=6416b15751cb

Comment 8

[Tom Tromey :tromey]

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.

Comment 9

[Tom Tromey :tromey]

Attachment: mark ToJSValue with MOZ_WARN_UNUSED_RESULT

Rebased.  I think this is ready now that the patch for bug 1101819 has
gone in.

Comment 10

[Tom Tromey :tromey]

https://tbpl.mozilla.org/?tree=Try&rev=70fdad857c9b

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8533995 [details] [diff] [review]
mark ToJSValue with MOZ_WARN_UNUSED_RESULT

r=me

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e25fb62cf2fd

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e25fb62cf2fd

Comment 14

[Lukas Blakk [:lsblakk] use ?needinfo]

Please nominate for beta/aurora uplift before Mon Dec 22 (for Beta, at least).

Comment 15

[Tom Tromey :tromey]

I don't think this one needs to be uplifted.
It is just a preventative measure against future issues like bug 1096329.