Closed
Bug 1342303
Opened 7 years ago
Closed 7 years ago
Add ranged-for loop support to nsCOMArray
Categories
(Core :: XPCOM, defect)
Core
XPCOM
Tracking
()
RESOLVED
FIXED
mozilla54
Tracking | Status | |
---|---|---|
firefox54 | --- | fixed |
People
(Reporter: xidorn, Assigned: xidorn)
References
Details
Attachments
(6 files)
59 bytes,
text/x-review-board-request
|
erahm
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
erahm
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
smaug
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
heycam
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
erahm
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
erahm
:
review+
|
Details |
No description provided.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 10•7 years ago
|
||
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 11•7 years ago
|
||
mozreview-review |
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+
Comment 12•7 years ago
|
||
(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.)
Assignee | ||
Comment 13•7 years ago
|
||
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.
Comment 14•7 years ago
|
||
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.
Assignee | ||
Comment 15•7 years ago
|
||
(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 16•7 years ago
|
||
mozreview-review |
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+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
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 23•7 years ago
|
||
mozreview-review |
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 24•7 years ago
|
||
mozreview-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 25•7 years ago
|
||
mozreview-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 26•7 years ago
|
||
mozreview-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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 33•7 years ago
|
||
mozreview-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+
Comment 34•7 years ago
|
||
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
I had to back these out for build bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=81303500&repo=autoland https://hg.mozilla.org/integration/autoland/rev/b0b40ce3dfe5
Flags: needinfo?(xidorn+moz)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 39•7 years ago
|
||
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
Assignee | ||
Updated•7 years ago
|
Flags: needinfo?(xidorn+moz)
Comment 40•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/6de42abcf2c7 https://hg.mozilla.org/mozilla-central/rev/b1df335e52e4 https://hg.mozilla.org/mozilla-central/rev/5460a7ea4e1f https://hg.mozilla.org/mozilla-central/rev/a6b68a941e55 https://hg.mozilla.org/mozilla-central/rev/2d861e4e01ef https://hg.mozilla.org/mozilla-central/rev/6e4c4e2e7147
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox54:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in
before you can comment on or make changes to this bug.
Description
•