Closed Bug 1237201 Opened 5 years ago Closed 5 years ago

Use MOZ_WARN_UNUSED_RESULT for fallible Vector methods


(Core :: MFBT, defect)

Not set



Tracking Status
firefox46 --- fixed


(Reporter: jandem, Assigned: jandem)




(9 files)

Vector is used mostly in js/src and I fixed most issues there in bug 1231224, but there are still warnings left in other parts of Gecko.
Depends on: 1237230
I just used MOZ_ALWAYS_TRUE here because there's a reserve(capacity) call before it.
Attachment #8704601 - Flags: review?(ydelendik)
Attached patch Part 2 - gfx/Splinter Review
I propagated OOM where that was possible. Other places I added a MOZ_CRASH.
Attachment #8704604 - Flags: review?(bugmail.mozilla)
Comment on attachment 8704601 [details] [diff] [review]
Part 1 - nsScriptLoader

Looks good. Thanks.
Attachment #8704601 - Flags: review?(ydelendik) → review+
Attachment #8704610 - Flags: review?(seth)
Comment on attachment 8704604 [details] [diff] [review]
Part 2 - gfx/

Review of attachment 8704604 [details] [diff] [review]:

The APZC changes look ok to me, but I'm not familiar with the other bits of code, so flagging jeff for review.
Attachment #8704604 - Flags: review?(jmuizelaar)
Attachment #8704604 - Flags: review?(bugmail.mozilla)
Attachment #8704604 - Flags: review+
Attached patch Part 4 - ipc/Splinter Review
Attachment #8704614 - Flags: review?(wmccloskey)
There's already a comment "silently ignore OOM errors", so this patch does that.
Attachment #8704627 - Flags: review?(n.nethercote)
Comment on attachment 8704614 [details] [diff] [review]
Part 4 - ipc/

Review of attachment 8704614 [details] [diff] [review]:

::: ipc/glue/MessageChannel.cpp
@@ +778,5 @@
>          for (MessageQueue::iterator it = mPending.begin(); it != mPending.end(); ) {
>              Message &msg = *it;
>              if (!ShouldDeferMessage(msg)) {
> +                if (!toProcess.append(OCMove(msg)))

Ugh, no idea where the "OC" came from. Please ignore that part.
Attachment #8704629 - Flags: review?(rjesup)
Attachment #8704629 - Flags: review?(rjesup) → review+
As discussed on IRC, I made most of them MOZ_CRASH and I'll file a follow-up bug to try to handle (some of) them more gracefully.

In some cases handling the OOM was straight-forward. We have to be careful not to leak anything on OOM so if there was any doubt that could happen I went with MOZ_CRASH.
Attachment #8704645 - Flags: review?(dteller)
Comment on attachment 8704645 [details] [diff] [review]
Part 7 - nsPerformanceStats, telemetry

Review of attachment 8704645 [details] [diff] [review]:

::: toolkit/components/telemetry/Telemetry.cpp
@@ +3671,5 @@
>    MutexAutoLock autoLock(sTelemetry->mThreadHangStatsMutex);
> +  // Ignore OOM.
> +  mozilla::Unused << sTelemetry->mThreadHangStats.append(Move(aStats));

I'm quite happy to r+ this line, but just to be clear, it's not my code.

::: toolkit/components/telemetry/ThreadHangStats.h
@@ +187,5 @@
>    void Add(PRIntervalTime aTime, HangMonitor::HangAnnotationsPtr aAnnotations) {
>      TimeHistogram::Add(aTime);
>      if (aAnnotations) {
> +      if (!mAnnotations.append(Move(aAnnotations))) {
> +        MOZ_CRASH();

This looks like it should fail silently. I'm willing to r+, but could you file a followup bug for each of ThreadHangStats and BackgroundHangMonitor?
Attachment #8704645 - Flags: review?(dteller) → review+
Adds MOZ_WARN_UNUSED_RESULT to all fallible methods in Vector.h
Attachment #8704670 - Flags: review?(jwalden+bmo)
Attachment #8704614 - Flags: review?(wmccloskey) → review+
Comment on attachment 8704604 [details] [diff] [review]
Part 2 - gfx/

Review of attachment 8704604 [details] [diff] [review]:

It's not clear to me why any of this code is using mozilla::Vector instead of std::vector. I expect some of them should just be using std::vector instead.
Attachment #8704627 - Flags: review?(n.nethercote) → review+
(In reply to Jeff Muizelaar [:jrmuizel] from comment #13)
> It's not clear to me why any of this code is using mozilla::Vector instead
> of std::vector. I expect some of them should just be using std::vector
> instead.

I can file a follow-up bug for this? I was hoping I could land this soon to avoid new failures, and the patch is simple and a clear improvement...
Comment on attachment 8704670 [details] [diff] [review]

Review of attachment 8704670 [details] [diff] [review]:
Attachment #8704670 - Flags: review?(jwalden+bmo) → review+
Comment on attachment 8704610 [details] [diff] [review]
Part 3 - StreamingLexer

According to firebot and bugzilla, Seth has been inactive for a while so forwarding the request to njn.
Attachment #8704610 - Flags: review?(seth) → review?(n.nethercote)
Attachment #8704604 - Flags: review?(jmuizelaar) → review+
Attachment #8704610 - Flags: review?(n.nethercote) → review+
Still need to land part 8.
Keywords: leave-open
I fixed all warnings I got locally with a Clang browser build, but Try uncovered some more issues:

* A (void) cast is enough to silence Clang but not GCC. I changed these to mozilla::Unused.

* There were some append() calls in SpiderMonkey's Profilers.cpp. We don't build this code by default so I didn't notice this before.

* I had to fix some append calls in Android/B2G code.
Attachment #8708036 - Flags: review?(nfroyd)
Attachment #8708036 - Flags: review?(nfroyd) → review+
Closed: 5 years ago
Keywords: leave-open
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
Depends on: 1356709
You need to log in before you can comment on or make changes to this bug.