1167418 - Handle return values of FallibleTArray functions in layout/

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Biru [:poiru]]

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.

layout/base/FrameLayerBuilder.cpp:5189:37:
  Tom Tromey: aRectangles.AppendElement(rect);

layout/base/SelectionCarets.cpp:1034:46:
  peter chang: states.AppendElement(SelectionState::Drag);

layout/base/SelectionCarets.cpp:1037:51:
  peter chang: states.AppendElement(SelectionState::Mousedown);

layout/base/SelectionCarets.cpp:1040:49:
  peter chang: states.AppendElement(SelectionState::Mouseup);

layout/base/SelectionCarets.cpp:1043:50:
  peter chang: states.AppendElement(SelectionState::Keypress);

layout/base/SelectionCarets.cpp:1046:51:
  peter chang: states.AppendElement(SelectionState::Selectall);

layout/base/SelectionCarets.cpp:1049:57:
  peter chang: states.AppendElement(SelectionState::Collapsetostart);

layout/base/SelectionCarets.cpp:1052:55:
  peter chang: states.AppendElement(SelectionState::Collapsetoend);

layout/base/SelectionCarets.cpp:1077:29:
  Ting-Yu Lin: state.AppendElement(aState);

layout/base/TouchCaret.cpp:1152:54:
  Morris Tseng: state.AppendElement(dom::SelectionState::Taponcaret);

layout/style/FontFaceSet.cpp:1546:55:
  Cameron McCormack: init.mFontfaces.AppendElements(aFontFaces.Length());

layout/style/MediaQueryList.cpp:106:38:
  Ehsan Akhgari: mCallbacks.AppendElement(&aListener);

layout/style/MediaQueryList.cpp:182:64:
  Ehsan Akhgari: HandleChangeData *d = aListenersToNotify.AppendElement();

Comment 1

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

For SelectionCarets.cpp and TouchCaret.cpp, I think we should use MOZ_ALWAYS_TRUE. Thanks.

Comment 2

[Cameron McCormack (:heycam)]

(In reply to Birunthan Mohanathas [:poiru] from comment #0)
> layout/style/FontFaceSet.cpp:1546:55:
>   Cameron McCormack: init.mFontfaces.AppendElements(aFontFaces.Length());

Just to confirm: this is an array that in other places can be set to a large size from web content with ease:

  var a = []; a.length = 100000000;
  new CSSFontFaceLoadEvent("blah", { fontfaces: a });

So this should be fallible (and is).

This AppendElements() call in FontFaceSet will attempt to append a web page controlled number of FontFace objects to the array, but the number of FontFaces is limited to a number determined by a linear amount of work by the page (e.g. inserting new FontFace objects one by one into document.fonts, or having a style sheet with the same number of @font-face rules).  So I think this can be fallible: does that sound right?  And if so, is MOZ_ASSERTing the return value the correct way to annotate that?

Comment 3

[Biru [:poiru]]

(In reply to Cameron McCormack (:heycam) from comment #2)
> This AppendElements() call in FontFaceSet will attempt to append a web page
> controlled number of FontFace objects to the array, but the number of
> FontFaces is limited to a number determined by a linear amount of work by
> the page (e.g. inserting new FontFace objects one by one into
> document.fonts, or having a style sheet with the same number of @font-face
> rules).  So I think this can be fallible: does that sound right?  And if so,
> is MOZ_ASSERTing the return value the correct way to annotate that?

Yes, it definitely should be fallible. MOZ_ASSERT/MOZ_ALWAYS_TRUE would be appropriate if it known that the call will always succeed (e.g. by having called SetCapacity previously). In this case, the call may well fail so I think the correct solution is:

  OwningNonNull<FontFace>* elements =
    init.mFontfaces.AppendElements(aFontFaces.Length());
  if (!elements) {
    return;
  }

or maybe:

  OwningNonNull<FontFace>* elements =
    init.mFontfaces.AppendElements(aFontFaces.Length());
  if (elements) {
    for (size_t i = 0; i < aFontFaces.Length(); i++) {
      elements[i] = aFontFaces[i];
    }
  }

Which do you think is right here?

Comment 4

[Cameron McCormack (:heycam)]

(In reply to Cameron McCormack (:heycam) from comment #2)
> This AppendElements() call in FontFaceSet will attempt to append a web page
> controlled number of FontFace objects to the array, but the number of
> FontFaces is limited to a number determined by a linear amount of work by
> the page (e.g. inserting new FontFace objects one by one into
> document.fonts, or having a style sheet with the same number of @font-face
> rules).  So I think this can be fallible: does that sound right?  And if so,
> is MOZ_ASSERTing the return value the correct way to annotate that?

Sorry, I meant to write "I think this can be infallible" there. :-)  Since you think it should be fallible, maybe my heuristics for determining whether it should be fallible are wrong?

If it should indeed be fallible, then returning early and not dispatching the event at all sounds fine to me.

Comment 5

[Cameron McCormack (:heycam)]

So let's say we have this API:

  interface NumberList {
    void append(double x);
    NumberList clone();
  };

where append just calls AppendElement() on an nsTArray, and clone() creates a new NumberList and copies its nsTArray contents over.

Can append's AppendElement() call be infallible, and if so, why?  If it can, then why can't clone() also be?

Comment 6

[Biru [:poiru]]

(In reply to Cameron McCormack (:heycam) from comment #4)
> Sorry, I meant to write "I think this can be infallible" there. :-)  Since
> you think it should be fallible, maybe my heuristics for determining whether
> it should be fallible are wrong?

AFAIK, we try to use fallible allocation when we are dealing with user input (as in this case).

Ehsan, since you made a few remarks about this over in bug 968520, do you have anything to add to this?

(In reply to Cameron McCormack (:heycam) from comment #5)
> So let's say we have this API:
> 
>   interface NumberList {
>     void append(double x);
>     NumberList clone();
>   };
> 
> where append just calls AppendElement() on an nsTArray, and clone() creates
> a new NumberList and copies its nsTArray contents over.
> 
> Can append's AppendElement() call be infallible, and if so, why?  If it can,
> then why can't clone() also be?

I'd say both should be fallible.

Comment 7

[Tom Tromey :tromey]

(In reply to Birunthan Mohanathas [:poiru] from comment #0)
> 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.
> 
> layout/base/FrameLayerBuilder.cpp:5189:37:
>   Tom Tromey: aRectangles.AppendElement(rect);

This is building a list of rectangles to attach to a timeline marker
for the devtools "new profiler".  I think it is fine in this case
to just ignore the error, as the user impact is pretty low, and 
because this is consistent with how errors seem to be dealt with
generally in the timeline code.

Comment 8

[(no longer active)]

(In reply to Birunthan Mohanathas [:poiru] from comment #6)
> (In reply to Cameron McCormack (:heycam) from comment #4)
> > Sorry, I meant to write "I think this can be infallible" there. :-)  Since
> > you think it should be fallible, maybe my heuristics for determining whether
> > it should be fallible are wrong?
> 
> AFAIK, we try to use fallible allocation when we are dealing with user input
> (as in this case).

It's a bit more nuanced.  If the memory that we're allocating is directly related to the size of the *input*, we typically don't try to allocate fallibly.  For example, if we're allocating an array large enough to contain every character in a page (or, every web font used in a page), then it makes sense for that allocation to be infallible, since for the page to OOM the browser, it would need to send something in the order of hundreds of millions bytes (or fonts) to the engine, and it's impossible to protect against that in a meaningful way.

However, if the page is having us allocate memory in a way that doesn't correspond linearly to the size of the input, we typically allocate fallibly to protect a page from OOMing us without sending us a large amount of data.

I think that the linear amount of input is very similar to the linear amount of work here.

> (In reply to Cameron McCormack (:heycam) from comment #5)
> > So let's say we have this API:
> > 
> >   interface NumberList {
> >     void append(double x);
> >     NumberList clone();
> >   };
> > 
> > where append just calls AppendElement() on an nsTArray, and clone() creates
> > a new NumberList and copies its nsTArray contents over.
> > 
> > Can append's AppendElement() call be infallible, and if so, why?  If it can,
> > then why can't clone() also be?
> 
> I'd say both should be fallible.

In this case, in order for append() to cause an OOM, the page needs to run it millions of times, and once it does that amount of work, it can be allocating the same amount of memory in many other ways too, so there is no point in using a fallible AppendElement() there.

clone() similarly requires a linear amount of work, so the same applies to it as well.

Comment 9

[Cameron McCormack (:heycam)]

Attachment: check fallible AppendElements call in FontFaceSet

Thanks Ehsan.  I'm going to go with MOZ_ASSERTing the AppendElements() return value.

Comment 10

[Biru [:poiru]]

Comment on attachment 8609704 [details] [diff] [review]
check fallible AppendElements call in FontFaceSet

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

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #8)
> It's a bit more nuanced.

Thanks for the clarification!

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1f9f41d4dbab

Comment 12

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

https://hg.mozilla.org/mozilla-central/rev/1f9f41d4dbab

Comment 13

[Biru [:poiru]]

Attachment: Use nsTArray instead of FallibleTArray in MediaQueryList

We already use the mozilla::fallible parameter when we want fallibility so
this doesn't actually change anything.

Comment 14

[Biru [:poiru]]

Attachment: Check AppendElement call in MediaQueryList

This preemptively silences an unused result warning.

Comment 15

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a87b8d5fa571
https://hg.mozilla.org/integration/mozilla-inbound/rev/afc53fc12e9e

Comment 16

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4a7a699e3fc2

Comment 17

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

https://hg.mozilla.org/mozilla-central/rev/a87b8d5fa571
https://hg.mozilla.org/mozilla-central/rev/afc53fc12e9e
https://hg.mozilla.org/mozilla-central/rev/4a7a699e3fc2