Handle return values of FallibleTArray functions in layout/

RESOLVED FIXED in mozilla42

Status

()

Core
Layout
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: poiru, Assigned: poiru)

Tracking

Trunk
mozilla42
Points:
---

Firefox Tracking Flags

(firefox41 affected)

Details

Attachments

(3 attachments)

(Assignee)

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.

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();
For SelectionCarets.cpp and TouchCaret.cpp, I think we should use MOZ_ALWAYS_TRUE. Thanks.
(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?
(Assignee)

Comment 3

3 years ago
(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?
Flags: needinfo?(cam)
(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.
Flags: needinfo?(cam)
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?
(Assignee)

Comment 6

3 years ago
(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.
Flags: needinfo?(ehsan)

Comment 7

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

3 years ago
(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.
Flags: needinfo?(ehsan)
Created attachment 8609704 [details] [diff] [review]
check fallible AppendElements call in FontFaceSet

Thanks Ehsan.  I'm going to go with MOZ_ASSERTing the AppendElements() return value.
Attachment #8609704 - Flags: review?(birunthan)
(Assignee)

Updated

3 years ago
Attachment #8609704 - Attachment is patch: true
(Assignee)

Comment 10

3 years ago
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!
Attachment #8609704 - Flags: review?(birunthan) → review+

Comment 11

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/1f9f41d4dbab
Keywords: leave-open
https://hg.mozilla.org/mozilla-central/rev/1f9f41d4dbab
(Assignee)

Comment 13

2 years ago
Created attachment 8632493 [details] [diff] [review]
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.
Attachment #8632493 - Flags: review?(cam)
(Assignee)

Updated

2 years ago
Assignee: nobody → birunthan
Status: NEW → ASSIGNED
(Assignee)

Comment 14

2 years ago
Created attachment 8632494 [details] [diff] [review]
Check AppendElement call in MediaQueryList

This preemptively silences an unused result warning.
Attachment #8632494 - Flags: review?(cam)
Attachment #8632493 - Flags: review?(cam) → review+
Attachment #8632494 - Flags: review?(cam) → review+

Comment 15

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

Comment 16

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/4a7a699e3fc2
https://hg.mozilla.org/mozilla-central/rev/a87b8d5fa571
https://hg.mozilla.org/mozilla-central/rev/afc53fc12e9e
https://hg.mozilla.org/mozilla-central/rev/4a7a699e3fc2
(Assignee)

Updated

2 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Keywords: leave-open
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
You need to log in before you can comment on or make changes to this bug.