1338272 - Error checking for MaybeSetPendingException in dom bindings (generated code)

Reporter

Description

•

7 years ago

From Coverity: 


** CID 1399676:  Error handling issues  (CHECKED_RETURN)
/obj-x86_64-pc-linux-gnu/dom/bindings/FlyWebWebSocketEventBinding.cpp: 125 in mozilla::dom::FlyWebWebSocketEventBinding::respondWith(JSContext *, JS::Handle<JSObject *>, mozilla::dom::FlyWebWebSocketEvent *, const JSJitMethodCallArgs &)()


________________________________________________________________________________________________________
*** CID 1399676:  Error handling issues  (CHECKED_RETURN)
/obj-x86_64-pc-linux-gnu/dom/bindings/FlyWebWebSocketEventBinding.cpp: 125 in mozilla::dom::FlyWebWebSocketEventBinding::respondWith(JSContext *, JS::Handle<JSObject *>, mozilla::dom::FlyWebWebSocketEvent *, const JSJitMethodCallArgs &)()
119         }
120         binding_detail::FastErrorResult promiseRv;
121         nsCOMPtr<nsIGlobalObject> global =
122           do_QueryInterface(promiseGlobal.GetAsSupports());
123         if (!global) {
124           promiseRv.Throw(NS_ERROR_UNEXPECTED);
>>>     CID 1399676:  Error handling issues  (CHECKED_RETURN)
>>>     Calling "MaybeSetPendingException" without checking return value (as is done elsewhere 3434 out of 3504 times).
125           promiseRv.MaybeSetPendingException(cx);
126           return false;
127         }
128         arg0 = Promise::Resolve(global, cx, valueToResolve,
129                                         promiseRv);
130         if (promiseRv.MaybeSetPendingException(cx)) {

** CID 1399677:  Error handling issues  (CHECKED_RETURN)
/obj-x86_64-pc-linux-gnu/dom/bindings/ExtendableEventBinding.cpp: 145 in mozilla::dom::ExtendableEventBinding::waitUntil(JSContext *, JS::Handle<JSObject *>, mozilla::dom::workers::ExtendableEvent *, const JSJitMethodCallArgs &)()


________________________________________________________________________________________________________
*** CID 1399677:  Error handling issues  (CHECKED_RETURN)
/obj-x86_64-pc-linux-gnu/dom/bindings/ExtendableEventBinding.cpp: 145 in mozilla::dom::ExtendableEventBinding::waitUntil(JSContext *, JS::Handle<JSObject *>, mozilla::dom::workers::ExtendableEvent *, const JSJitMethodCallArgs &)()
139         }
140         binding_detail::FastErrorResult promiseRv;
141         nsCOMPtr<nsIGlobalObject> global =
142           do_QueryInterface(promiseGlobal.GetAsSupports());
143         if (!global) {
144           promiseRv.Throw(NS_ERROR_UNEXPECTED);
>>>     CID 1399677:  Error handling issues  (CHECKED_RETURN)
>>>     Calling "MaybeSetPendingException" without checking return value (as is done elsewhere 3434 out of 3504 times).
145           promiseRv.MaybeSetPendingException(cx);
146           return false;
147         }
148         arg0 = Promise::Resolve(global, cx, valueToResolve,
149                                         promiseRv);
150         if (promiseRv.MaybeSetPendingException(cx)) {

** CID 1399678:  Error handling issues  (CHECKED_RETURN)
/obj-x86_64-pc-linux-gnu/dom/bindings/FlyWebFetchEventBinding.cpp: 82 in mozilla::dom::FlyWebFetchEventBinding::respondWith(JSContext *, JS::Handle<JSObject *>, mozilla::dom::FlyWebFetchEvent *, const JSJitMethodCallArgs &)()


________________________________________________________________________________________________________
*** CID 1399678:  Error handling issues  (CHECKED_RETURN)
/obj-x86_64-pc-linux-gnu/dom/bindings/FlyWebFetchEventBinding.cpp: 82 in mozilla::dom::FlyWebFetchEventBinding::respondWith(JSContext *, JS::Handle<JSObject *>, mozilla::dom::FlyWebFetchEvent *, const JSJitMethodCallArgs &)()
76         }
77         binding_detail::FastErrorResult promiseRv;
78         nsCOMPtr<nsIGlobalObject> global =
79           do_QueryInterface(promiseGlobal.GetAsSupports());
80         if (!global) {
81           promiseRv.Throw(NS_ERROR_UNEXPECTED);
>>>     CID 1399678:  Error handling issues  (CHECKED_RETURN)
>>>     Calling "MaybeSetPendingException" without checking return value (as is done elsewhere 3434 out of 3504 times).
82           promiseRv.MaybeSetPendingException(cx);
83           return false;
84         }
85         arg0 = Promise::Resolve(global, cx, valueToResolve,
86                                         promiseRv);
87         if (promiseRv.MaybeSetPendingException(cx)) {

** CID 1399679:  Error handling issues  (CHECKED_RETURN)
/obj-x86_64-pc-linux-gnu/dom/bindings/FetchEventBinding.cpp: 327 in mozilla::dom::FetchEventBinding::respondWith(JSContext *, JS::Handle<JSObject *>, mozilla::dom::workers::FetchEvent *, const JSJitMethodCallArgs &)()


________________________________________________________________________________________________________
*** CID 1399679:  Error handling issues  (CHECKED_RETURN)
/obj-x86_64-pc-linux-gnu/dom/bindings/FetchEventBinding.cpp: 327 in mozilla::dom::FetchEventBinding::respondWith(JSContext *, JS::Handle<JSObject *>, mozilla::dom::workers::FetchEvent *, const JSJitMethodCallArgs &)()
321         }
322         binding_detail::FastErrorResult promiseRv;
323         nsCOMPtr<nsIGlobalObject> global =
324           do_QueryInterface(promiseGlobal.GetAsSupports());
325         if (!global) {
326           promiseRv.Throw(NS_ERROR_UNEXPECTED);
>>>     CID 1399679:  Error handling issues  (CHECKED_RETURN)
>>>     Calling "MaybeSetPendingException" without checking return value (as is done elsewhere 3434 out of 3504 times).
327           promiseRv.MaybeSetPendingException(cx);
328           return false;
329         }
330         arg0 = Promise::Resolve(global, cx, valueToResolve,
331                                         promiseRv);
332         if (promiseRv.MaybeSetPendingException(cx)) {

Andrew Overholt [:overholt]

Comment 1

•

7 years ago

Boris, do you know what's up here?

Flags: needinfo?(bzbarsky)

Boris Zbarsky [:bzbarsky]

Comment 2

•

7 years ago

Sure.  MaybeSetPendingException() returns exactly the same value as Failed().  The idea is that instead of writing out:

  if (rv.Failed()) {
    rv.SetPendingException(cx);
    return;
  }

you do:

  if (rv.MaybeSetPendingException(cx)) {
    return;
  }

In the code quoted above, we _KNOW_ promiseRv.Failed() is true: we just called Throw() on it.  So there's no point to checking the return value of MaybeSetPendingException.

I _could_ make SetPendingException public and use it here, but it's too easy to misuse, so I'd rather not.

Flags: needinfo?(bzbarsky)

Andrew Overholt [:overholt]

Comment 3

•

7 years ago

Thanks. Sounds like a false positive or at least a P3 for us to change anything.

Priority: -- → P3

Andrew McCreight [:mccr8]

Assignee

Updated

•

7 years ago

Assignee: nobody → continuation

Blocks: coverity-analysis

Andrew McCreight [:mccr8]

Assignee

Comment 4

•

7 years ago

I have a patch that makes this method MOZ_MUST_USE and fixes a couple of places, including CodeGen, that ignore the return value. They all follow a throw in the same way.

Boris Zbarsky [:bzbarsky]

Comment 5

•

7 years ago

Maybe we should simply have a better API on ErrorResult for "throw this and immediately stick it on the given JSContext".

Andrew McCreight [:mccr8]

Assignee

Comment 6

•

7 years ago

(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #5)
> Maybe we should simply have a better API on ErrorResult for "throw this and
> immediately stick it on the given JSContext".

There are few enough of these I don't think it is worth the effort to come up with a new API.

Comment hidden (mozreview-request)

Most of the time, the return value of this method should be checked,
because behavior should depend on whether or not an exception is
thrown. However, if it is called immediately after a throw it doesn't
need to be checked because it will always return true. bz said there
is no public API that lets you assume there is an exception because it
would be "too easy to misuse".

Review commit: https://reviewboard.mozilla.org/r/119278/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/119278/

Andrew McCreight [:mccr8]

Assignee

Comment 8

•

7 years ago

opt and debug try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=312ce1e96a4c5b32fb76be4a43368b2bbed96955

Feel free to grab the review, Boris. I didn't ask because your review requests are closed.

Andrew McCreight [:mccr8]

Assignee

Updated

•

7 years ago

Attachment #8846315 - Flags: review?(peterv) → review?(bzbarsky)

Boris Zbarsky [:bzbarsky]

Comment 9

•

7 years ago

mozreview-review

Comment on attachment 8846315 [details]
Bug 1338272 - Require that the return value of MaybeSetPendingException is used.

https://reviewboard.mozilla.org/r/119278/#review131742

::: commit-message-d18cc:1
(Diff revision 1)
> +Bug 1338272 - Require that the return value of MaybeSetPendingException is used. r=peterv

Fix the reviewer here?  ;)

Attachment #8846315 - Flags: review?(bzbarsky) → review+

Andrew McCreight [:mccr8]

Assignee

Comment 10

•

7 years ago

(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #9)
> Fix the reviewer here?  ;)

I think mozreview actually fixes that up.

Pulsebot

Comment 11

•

7 years ago

Pushed by amccreight@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5505b53a0acb
Require that the return value of MaybeSetPendingException is used. r=bz

Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)

Comment 12

•

7 years ago

bugherder

https://hg.mozilla.org/mozilla-central/rev/5505b53a0acb

Status: NEW → RESOLVED

Closed: 7 years ago

status-firefox55: --- → fixed

Resolution: --- → FIXED

Target Milestone: --- → mozilla55

Nobody; OK to take it and work on it

Updated

•

5 years ago

Component: DOM → DOM: Core & HTML

Bugzilla

Quick Search

Error checking for MaybeSetPendingException in dom bindings (generated code)

Categories

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

Tracking

()

People

(Reporter: jesup, Assigned: mccr8)

References

(Blocks 1 open bug)

Details

Crash Data

Security

(public)

User Story

Attachments

(1 file)

Description

Comment 1

Comment 2

Comment 3

Updated

Comment 4

Comment 5

Comment 6

Comment 7

Comment 8

Updated

Comment 9

Comment 10

Comment 11

Comment 12

Updated

Attachment

General

Description

File Name

Content Type