Closed Bug 1167423 Opened 5 years ago Closed 5 years ago

Handle return values of FallibleTArray functions in dom/

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox41 --- fixed

People

(Reporter: poiru, Assigned: baku)

References

(Blocks 1 open bug)

Details

Attachments

(10 files, 1 obsolete file)

Below is a list of unchecked fallible calls. After bug 968520 lands, these will cause 'unused result' warnings. Could you please let me know how you'd like them fixed (e.g. ignore result, use MOZ_ASSERT/MOZ_ENSURE_TRUE, propagate error)? Feel free to fix them yourself.

dom/base/Console.cpp:1430:49:
  Dmitry Sagalovskiy: aSequence.AppendElement(JS::StringValue(str));

dom/base/Console.cpp:1555:34:
  Andrea Marchesini: aSequence.AppendElement(v);

dom/base/Console.cpp:1573:52:
  Mihai Sucan: aStyles.AppendElement(JS::NullValue());

dom/base/Console.cpp:1576:58:
  Mihai Sucan: aStyles.AppendElement(JS::StringValue(jsString));

dom/base/Console.cpp:1644:41:
  Andrea Marchesini: aSequence.AppendElement(aData[index]);

dom/base/Console.cpp:1776:37:
  Andrea Marchesini: aSequence.AppendElement(aData[i]);

dom/base/Console.cpp:669:36:
  Andrea Marchesini: arguments.AppendElement(value);

dom/base/Console.cpp:830:29:
  Andrea Marchesini: data.AppendElement(aTime);

dom/base/Console.cpp:843:29:
  Andrea Marchesini: data.AppendElement(aTime);

dom/base/Console.cpp:856:29:
  Jordan Santell: data.AppendElement(aData);

dom/base/Console.cpp:895:36:
  Andrea Marchesini: sequence.AppendElement(aData[i]);

dom/base/WebSocket.cpp:963:36:
  Andrea Marchesini: protocols.AppendElement(aProtocol);

dom/base/nsDOMMutationObserver.cpp:715:73:
  Olli Pettay: filtersAsStrings.AppendElement(nsDependentAtomString(filters[j]));

dom/canvas/CanvasRenderingContext2D.cpp:2677:25:
  Rik Cabanier: dash.AppendElement(1);

dom/canvas/CanvasRenderingContext2D.cpp:2678:25:
  Rik Cabanier: dash.AppendElement(1);

dom/canvas/CanvasRenderingContext2D.cpp:3960:36:
  Rik Cabanier: dash.AppendElement(aSegments[x]);

dom/canvas/CanvasRenderingContext2D.cpp:3964:38:
  Rik Cabanier: dash.AppendElement(aSegments[x]);

dom/canvas/WebGL2ContextFramebuffers.cpp:352:62:
  David Anderson: out->AppendElement(LOCAL_GL_COLOR_ATTACHMENT0);

dom/canvas/WebGL2ContextFramebuffers.cpp:355:61:
  David Anderson: out->AppendElement(LOCAL_GL_DEPTH_ATTACHMENT);

dom/canvas/WebGL2ContextFramebuffers.cpp:358:63:
  David Anderson: out->AppendElement(LOCAL_GL_STENCIL_ATTACHMENT);

dom/crypto/WebCryptoTask.cpp:1967:56:
  Richard Barnes: mJwk.mKey_ops.Value().AppendElements(mKeyUsages);

dom/datastore/DataStoreDB.cpp:319:69:
  Ben Turner: objectStores.RawSetAsStringSequence().AppendElements(mObjectStores);

dom/datastore/DataStoreService.cpp:1361:60:
  Andrea Marchesini: dbs.AppendElement(NS_LITERAL_STRING(DATASTOREDB_REVISION));

dom/html/HTMLInputElement.cpp:1863:32:
  Andrea Marchesini: list.AppendElement(aValue);

dom/html/HTMLInputElement.cpp:2444:56:
  Andrea Marchesini: list.AppendElement(nsDependentString(aFileNames[i]));

dom/html/HTMLInputElement.cpp:2495:30:
  Andrea Marchesini: list.AppendElement(aValue);

dom/media/mediasource/ResourceQueue.cpp:115:58:
  Chris Double: data->AppendElements(item->mData->Elements() + offset,

dom/mobilemessage/MobileMessageManager.cpp:602:71:
  Bevis Tseng: deletedMsgIds.AppendElements(info->GetData().deletedMessageIds());

dom/mobilemessage/MobileMessageManager.cpp:608:73:
  Bevis Tseng: deletedThreadIds.AppendElements(info->GetData().deletedThreadIds());

dom/tv/TVSource.cpp:352:29:
  Sean Lin: *programs.AppendElement() = program;
(In reply to Birunthan Mohanathas [:poiru] from comment #0)
> dom/base/nsDOMMutationObserver.cpp:715:73:
>   Olli Pettay:
> filtersAsStrings.AppendElement(nsDependentAtomString(filters[j]));
The best option would be to mark
http://mxr.mozilla.org/mozilla-central/source/dom/webidl/MutationObserver.webidl?rev=98a32495dbb1&mark=46-47#47
[ChromeOnly,Throws] and then nsDOMMutationObserver::GetObservingInfo gets ErrorResult& as the second param, and pass NS_ERROR_OUT_OF_MEMORY to it.
Attachment #8609290 - Flags: review?(bugs)
Assignee: nobody → amarchesini
Attached patch c2.patch (obsolete) — Splinter Review
Attachment #8609294 - Flags: review?(bugs)
Attachment #8609294 - Attachment is obsolete: true
Attachment #8609294 - Flags: review?(bugs)
Attachment #8609298 - Flags: review?(bugs)
Attachment #8609301 - Flags: review?(bugs)
Attachment #8609303 - Flags: review?(bugs)
Attachment #8609308 - Flags: review?(bugs)
Attachment #8609313 - Flags: review?(bugs)
Attachment #8609316 - Flags: review?(bugs)
Attachment #8609317 - Flags: review?(bugs)
Attachment #8609318 - Flags: review?(bugs)
Attachment #8609319 - Flags: review?(bugs)
Attachment #8609290 - Flags: review?(bugs) → review+
Attachment #8609298 - Flags: review?(bugs) → review+
Comment on attachment 8609301 [details] [diff] [review]
patch 3 - MutationObserver

>-nsDOMMutationObserver::GetObservingInfo(nsTArray<Nullable<MutationObservingInfo> >& aResult)
>+nsDOMMutationObserver::GetObservingInfo(
>+                             nsTArray<Nullable<MutationObservingInfo>>& aResult,
>+                             mozilla::ErrorResult& aRv)
a bit odd indentation. Perhaps 
>+nsDOMMutationObserver::GetObservingInfo(
>+                         nsTArray<Nullable<MutationObservingInfo>>& aResult,
>+                         mozilla::ErrorResult& aRv)
though that doesn't look too good either (but would at least use 2 spaces from something).
Either way.
Attachment #8609301 - Flags: review?(bugs) → review+
Attachment #8609319 - Flags: review?(bugs) → review+
I'm not sure about adding [Throws] to .webidl in case the API is non-ChromeOnly.
Attachment #8609316 - Flags: review?(bugs) → review+
Attachment #8609317 - Flags: review?(bugs) → review+
Attachment #8609313 - Flags: review?(bugs) → review+
Comment on attachment 8609318 [details] [diff] [review]
patch 9 - MediaSource

(I don't know what behavior is wanted here.)
Attachment #8609318 - Flags: review?(bugs) → review?(jyavenard)
Comment on attachment 8609303 [details] [diff] [review]
patch 4 - CanvasRenderingContext2D

>+      if (!dash.AppendElement(aSegments[x])) {
>+      aRv.Throw(NS_ERROR_OUT_OF_MEMORY);
missing indentation


though, I'm not really agains throws.
Attachment #8609303 - Flags: review?(bugs) → review+
Attachment #8609308 - Flags: review?(bugs) → review+
Comment on attachment 8609318 [details] [diff] [review]
patch 9 - MediaSource

Review of attachment 8609318 [details] [diff] [review]:
-----------------------------------------------------------------

nice catch.

We need to report the error to the owning's sourcebuffer. There's no easy implementation nor do I think the MSE spec cater for this (appending to a source buffer causing another sourcebuffer to abort). But seeing that particular eviction isn't currently working and that code isn't called. will be fine for now.

Please open a follow up bug to handle this.
thanks

::: dom/media/mediasource/TrackBuffer.cpp
@@ +347,4 @@
>            toEvict -= decoders[i]->GetResource()->EvictData(playbackOffset,
> +                                                           playbackOffset,
> +                                                           rv);
> +          if (NS_WARN_IF(rv.Failed())) { 

trailing space

@@ +500,5 @@
> +      mInitializedDecoders[i]->GetResource()->EvictBefore(endOffset, rv);
> +      if (NS_WARN_IF(rv.Failed())) {
> +        rv.SuppressException();
> +        return;
> +      }

this will leave data in an unplayable state, we need a way to report that error back to the owning element and properly throw the error to the owning sourcebuffer.
Attachment #8609318 - Flags: review?(jyavenard) → review+
Blocks: 1168080
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.