Handle return values of FallibleTArray functions in dom/

RESOLVED FIXED in Firefox 41

Status

()

Core
DOM
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: poiru, Assigned: baku)

Tracking

(Blocks: 1 bug)

Trunk
mozilla41
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox41 fixed)

Details

Attachments

(10 attachments, 1 obsolete attachment)

(Reporter)

Description

3 years ago
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.
(Assignee)

Comment 2

3 years ago
Created attachment 8609290 [details] [diff] [review]
patch 0 - Console API
Attachment #8609290 - Flags: review?(bugs)
(Assignee)

Updated

3 years ago
Assignee: nobody → amarchesini
(Assignee)

Comment 3

3 years ago
Created attachment 8609294 [details] [diff] [review]
c2.patch
Attachment #8609294 - Flags: review?(bugs)
(Assignee)

Comment 4

3 years ago
Created attachment 8609298 [details] [diff] [review]
patch 1 - WebSocket
Attachment #8609294 - Attachment is obsolete: true
Attachment #8609294 - Flags: review?(bugs)
Attachment #8609298 - Flags: review?(bugs)
(Assignee)

Comment 5

3 years ago
Created attachment 8609301 [details] [diff] [review]
patch 3 - MutationObserver
Attachment #8609301 - Flags: review?(bugs)
(Assignee)

Comment 6

3 years ago
Created attachment 8609303 [details] [diff] [review]
patch 4 - CanvasRenderingContext2D
Attachment #8609303 - Flags: review?(bugs)
(Assignee)

Comment 7

3 years ago
Created attachment 8609308 [details] [diff] [review]
patch 5 - WebGL2Context
Attachment #8609308 - Flags: review?(bugs)
(Assignee)

Comment 8

3 years ago
Created attachment 8609313 [details] [diff] [review]
patch 6 - WebCryptoTask
Attachment #8609313 - Flags: review?(bugs)
(Assignee)

Comment 9

3 years ago
Created attachment 8609316 [details] [diff] [review]
patch 7 - DataStore API
Attachment #8609316 - Flags: review?(bugs)
(Assignee)

Comment 10

3 years ago
Created attachment 8609317 [details] [diff] [review]
patch 8 - HTMLInputElement
Attachment #8609317 - Flags: review?(bugs)
(Assignee)

Comment 11

3 years ago
Created attachment 8609318 [details] [diff] [review]
patch 9 - MediaSource
Attachment #8609318 - Flags: review?(bugs)
(Assignee)

Comment 12

3 years ago
Created attachment 8609319 [details] [diff] [review]
patch 10 - mobilemessage
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+
(Assignee)

Updated

3 years ago
Blocks: 1168080
You need to log in before you can comment on or make changes to this bug.