Closed Bug 1342303 Opened 3 years ago Closed 3 years ago

Add ranged-for loop support to nsCOMArray

Categories

(Core :: XPCOM, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox54 --- fixed

People

(Reporter: xidorn, Assigned: xidorn)

References

Details

Attachments

(6 files)

No description provided.
yikes, my one of all time favored "crash browser, hopefully not unsafely", ranged-for :)
We have had tons of bugs because of ranged-for loops.

Be careful, very very careful with loops. Even if ranged-for shouldn't be unsafe anymore with TArray, it just crashes the browser if one does something bad. And at least very recently we certainly had web exposed code prone to such crashes.
Comment on attachment 8840817 [details]
Bug 1342303 part 3 - Remove nsCOMArray::EnumerateForwards uses in docshell.

https://reviewboard.mozilla.org/r/115252/#review116692
Attachment #8840817 - Flags: review?(bugs) → review+
(I do occasionally think that ranged-for is so error prone construct that we should remove support for it in nsTArray and elsewhere. But I'm sure others would object that.)
I don't think range-based for loop can be any more error-prone than the old EnumerateForwards, especially given that the iterator is now index-based rather than pointer-based.
EnumerateForwards is very safe, since it always checks that it doesn't index out-of-bounds.
So, at least it doesn't crash, like ranged-for: http://searchfox.org/mozilla-central/rev/60ae6514e4c559c0c234f0e7aefccb101b8beb2e/xpcom/ds/nsTArray.h#360-370

Miss-behaving is possible with both if array is modified.
(In reply to Olli Pettay [:smaug] (pto-ish for couple of days) from comment #14)
> EnumerateForwards is very safe, since it always checks that it doesn't index
> out-of-bounds.
> So, at least it doesn't crash, like ranged-for:
> http://searchfox.org/mozilla-central/rev/
> 60ae6514e4c559c0c234f0e7aefccb101b8beb2e/xpcom/ds/nsTArray.h#360-370
> 
> Miss-behaving is possible with both if array is modified.

Oh, hmmm, okay. nsCOMArray would reuse the iterator of nsTArray, and inherit its behavior. Yes it may crash when the array is modified. I think we should always fix that misbehavior rather than hiding that behind a safe method. And EnumerateForwards could be much slower than range-based for loop, since it needs to do a function call for each item.
Comment on attachment 8840818 [details]
Bug 1342303 part 4 - Remove nsCOMArray::EnumerateForwards uses in layout/style.

https://reviewboard.mozilla.org/r/115254/#review116698

::: layout/style/CSSStyleSheet.cpp:280
(Diff revision 2)
> -    mOrderedRules.EnumerateForwards(SetStyleSheetReference,
> -                                    mSheets.ElementAt(0));
> +    CSSStyleSheet* sheet = mSheets.ElementAt(0);
> +    for (css::Rule* rule : mOrderedRules) {
> +      rule->SetStyleSheet(sheet);
> +    }

You'll conflict with the bug 1290218 changes here.

::: layout/style/CSSStyleSheet.cpp:311
(Diff revision 2)
> +      int32_t type = rule->GetType();
> +      if (css::Rule::NAMESPACE_RULE == type) {
> +        AddNamespaceRuleToMap(rule, mNameSpaceMap);
> +      } else if (type != css::Rule::CHARSET_RULE &&
> +                 type != css::Rule::IMPORT_RULE) {

Nit: this might look nicer as a switch statement.
Attachment #8840818 - Flags: review?(cam) → review+
Attachment #8840815 - Flags: review?(nfroyd) → review?(erahm)
Attachment #8840816 - Flags: review?(nfroyd) → review?(erahm)
Attachment #8840819 - Flags: review?(nfroyd) → review?(erahm)
Attachment #8840820 - Flags: review?(nfroyd) → review?(erahm)
Comment on attachment 8840815 [details]
Bug 1342303 part 1 - Make nsTArrayIterator an independent class.

https://reviewboard.mozilla.org/r/115248/#review117944

::: mfbt/ArrayIterator.h:81
(Diff revision 2)
> +  bool operator>=(const iterator_type& aRhs) const {
> +    return mIndex >= aRhs.mIndex;
> +  }
> +
> +  // These operators depend on the release mode bounds checks in
> +  // ArrayIterator::ElementAt for safety.

Now that this is moving out of nsTArray.h it seems liek we should probably enforce this explicitly, so call `mArray->ElementAt` rather than `mArray->operator[]`

::: mfbt/moz.build:21
(Diff revision 2)
>  EXPORTS.mozilla = [
>      'Alignment.h',
>      'AllocPolicy.h',
>      'AlreadyAddRefed.h',
>      'Array.h',
> +    'ArrayIterator.h',

Two concerns:
  1. I'm not an mfbt peer ;)
  2. I don't think this belongs in mfbt, the first point is probably okay

It's confusing having a 'mfbt/ArrayIterator.h' next to 'mfbt/Array.h' which doesn't use it. Can you just leave it under xpcom and install it to 'mozilla/' with 'EXPORTS.mozilla'?

Otherwise can you find a mfbt peer for this part?
Attachment #8840815 - Flags: review?(erahm) → review-
Comment on attachment 8840816 [details]
Bug 1342303 part 2 - Add range-based for loop support to nsCOMArray.

https://reviewboard.mozilla.org/r/115250/#review117956

r=me
Attachment #8840816 - Flags: review?(erahm) → review+
Comment on attachment 8840819 [details]
Bug 1342303 part 5 - Remove nsCOMArray::EnumerateForwards uses in xpfe.

https://reviewboard.mozilla.org/r/115256/#review117960

Nice cleanup.
Attachment #8840819 - Flags: review?(erahm) → review+
Comment on attachment 8840820 [details]
Bug 1342303 part 6 - Remove nsCOMArray::Enumerate{Forwards,Backwards}.

https://reviewboard.mozilla.org/r/115258/#review117962

r=me
Attachment #8840820 - Flags: review?(erahm) → review+
Comment on attachment 8840815 [details]
Bug 1342303 part 1 - Make nsTArrayIterator an independent class.

https://reviewboard.mozilla.org/r/115248/#review118374

r=me, thanks for the update.
Attachment #8840815 - Flags: review?(erahm) → review+
Pushed by xquan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b5de1dfff82f
part 1 - Make nsTArrayIterator an independent class. r=erahm
https://hg.mozilla.org/integration/autoland/rev/5c6042dee665
part 2 - Add range-based for loop support to nsCOMArray. r=erahm
https://hg.mozilla.org/integration/autoland/rev/bdc491b9ebde
part 3 - Remove nsCOMArray::EnumerateForwards uses in docshell. r=smaug
https://hg.mozilla.org/integration/autoland/rev/bc3b2e7a383b
part 4 - Remove nsCOMArray::EnumerateForwards uses in layout/style. r=heycam
https://hg.mozilla.org/integration/autoland/rev/20a1bcb47c33
part 5 - Remove nsCOMArray::EnumerateForwards uses in xpfe. r=erahm
https://hg.mozilla.org/integration/autoland/rev/89137679a68c
part 6 - Remove nsCOMArray::Enumerate{Forwards,Backwards}. r=erahm
Pushed by xquan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6de42abcf2c7
part 1 - Make nsTArrayIterator an independent class. r=erahm
https://hg.mozilla.org/integration/autoland/rev/b1df335e52e4
part 2 - Add range-based for loop support to nsCOMArray. r=erahm
https://hg.mozilla.org/integration/autoland/rev/5460a7ea4e1f
part 3 - Remove nsCOMArray::EnumerateForwards uses in docshell. r=smaug
https://hg.mozilla.org/integration/autoland/rev/a6b68a941e55
part 4 - Remove nsCOMArray::EnumerateForwards uses in layout/style. r=heycam
https://hg.mozilla.org/integration/autoland/rev/2d861e4e01ef
part 5 - Remove nsCOMArray::EnumerateForwards uses in xpfe. r=erahm
https://hg.mozilla.org/integration/autoland/rev/6e4c4e2e7147
part 6 - Remove nsCOMArray::Enumerate{Forwards,Backwards}. r=erahm
Flags: needinfo?(xidorn+moz)
Depends on: 1344133
Depends on: 1344544
You need to log in before you can comment on or make changes to this bug.