1645339 - Use range-based for with nsTObserverArray where easily possible

Status: RESOLVED FIXED

(Core :: XPCOM, task)

Comment 1

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1645339 - Use range-based for with nsTObserverArray in dom/cache. r=#dom-workers-and-storage

Depends on D79284

Comment 2

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1645339 - Use range-based for with nsTObserverArray in dom/serviceworkers. r=#dom-workers-and-storage

Depends on D79431

Comment 3

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1645339 - Use range-based for with nsTObserverArray in dom/workers. r=#dom-workers-and-storage

Depends on D79432

Comment 4

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1645339 - Use range-based for with nsTObserverArray in dom/storage. r=#dom-workers-and-storage

Depends on D79433

Comment 5

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1645339 - Use range-based for with nsTObserverArray in xpcom. r=#xpcom-reviewers

Depends on D79434

Comment 6

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1645339 - Use range-based for with nsTObserverArray in docshell. r=nika

Depends on D79490

Comment 7

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1645339 - Use range-based for with nsTObserverArray in accessible. r=MarcoZ

Depends on D79493

Comment 8

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1645339 - Use range-based for with nsTObserverArray in dom/abort. r=smaug

Depends on D79494

Comment 9

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1645339 - Use range-based for with nsTObserverArray in dom/audiochannel. r=baku

Depends on D79495

Comment 10

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1645339 - Use range-based for with nsTObserverArray in layout/base. r=farre

Depends on D79496

Comment 11

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1645339 - Use range-based for with nsTObserverArray in layout/generic. r=tnikkel

Depends on D79497

Comment 12

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1645339 - Use range-based for with nsTObserverArray in dom/base. r=nika

Depends on D79498

Comment 13

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1645339 - Use range-based for with nsTObserverArray in dom/debugger. r=jimb

Depends on D79499

Comment 14

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1645339 - Use range-based for with nsTObserverArray in dom/events. r=smaug

Depends on D79500

Comment 15

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1645339 - Use range-based for with nsTObserverArray in dom/performance. r=baku

Depends on D79501

Comment 16

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1645339 - Use range-based for with nsTObserverArray in dom/reporting. r=smaug

Depends on D79502

Comment 17

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1645339 - Use range-based for with nsTObserverArray in mobile/android. r=mak

Depends on D79503

Comment 18

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1645339 - Use range-based for with nsTObserverArray in xpfe. r=smaug

Depends on D79504

Comment 19

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1645339 - Use range-based for with nsTObserverArray in layout/style. r=heycam

Depends on D79505

Comment 20

[Olli Pettay [:smaug][[e-mail]]]

The bug could explain why? :)

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5329613a793b
Use range-based for with nsTObserverArray in xpcom. r=froydnj

Comment 22

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/5329613a793b

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e6239a2ea63d
Use range-based for with nsTObserverArray in docshell. r=nika
https://hg.mozilla.org/integration/autoland/rev/9e423eab275c
Use range-based for with nsTObserverArray in accessible. r=MarcoZ
https://hg.mozilla.org/integration/autoland/rev/ea0b74e44d75
Use range-based for with nsTObserverArray in dom/abort. r=smaug
https://hg.mozilla.org/integration/autoland/rev/c8ee4717dac6
Use range-based for with nsTObserverArray in dom/audiochannel. r=baku
https://hg.mozilla.org/integration/autoland/rev/91138138812e
Use range-based for with nsTObserverArray in layout/generic. r=tnikkel
https://hg.mozilla.org/integration/autoland/rev/490fa07526ce
Use range-based for with nsTObserverArray in dom/events. r=smaug
https://hg.mozilla.org/integration/autoland/rev/346a1bc55d57
Use range-based for with nsTObserverArray in dom/performance. r=baku
https://hg.mozilla.org/integration/autoland/rev/d091f09fb8ad
Use range-based for with nsTObserverArray in dom/reporting. r=smaug
https://hg.mozilla.org/integration/autoland/rev/a3218470c558
Use range-based for with nsTObserverArray in mobile/android. r=geckoview-reviewers,agi
https://hg.mozilla.org/integration/autoland/rev/756fe40189b7
Use range-based for with nsTObserverArray in xpfe. r=smaug

Comment 24

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/e6239a2ea63d
https://hg.mozilla.org/mozilla-central/rev/9e423eab275c
https://hg.mozilla.org/mozilla-central/rev/ea0b74e44d75
https://hg.mozilla.org/mozilla-central/rev/c8ee4717dac6
https://hg.mozilla.org/mozilla-central/rev/91138138812e
https://hg.mozilla.org/mozilla-central/rev/490fa07526ce
https://hg.mozilla.org/mozilla-central/rev/346a1bc55d57
https://hg.mozilla.org/mozilla-central/rev/d091f09fb8ad
https://hg.mozilla.org/mozilla-central/rev/a3218470c558
https://hg.mozilla.org/mozilla-central/rev/756fe40189b7

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/73823aa5538b
Use range-based for with nsTObserverArray in layout/base. r=tnikkel
https://hg.mozilla.org/integration/autoland/rev/e86ae0d0b516
Use range-based for with nsTObserverArray in dom/cache. r=dom-workers-and-storage-reviewers,edenchuang
https://hg.mozilla.org/integration/autoland/rev/448405bd15a2
Use range-based for with nsTObserverArray in dom/serviceworkers. r=dom-workers-and-storage-reviewers,edenchuang
https://hg.mozilla.org/integration/autoland/rev/780ae7d7aba4
Use range-based for with nsTObserverArray in dom/workers. r=dom-workers-and-storage-reviewers,edenchuang
https://hg.mozilla.org/integration/autoland/rev/0d6545fa7762
Use range-based for with nsTObserverArray in dom/storage. r=dom-workers-and-storage-reviewers,edenchuang

Comment 26

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/73823aa5538b
https://hg.mozilla.org/mozilla-central/rev/e86ae0d0b516
https://hg.mozilla.org/mozilla-central/rev/448405bd15a2
https://hg.mozilla.org/mozilla-central/rev/780ae7d7aba4
https://hg.mozilla.org/mozilla-central/rev/0d6545fa7762

Comment 27

[Andrew Sutherland [:asuth] (he/him)]

(In reply to Olli Pettay [:smaug] from comment #20)

The bug could explain why? :)

Did this get answered somewhere? (I should have needinfo'ed previously when deciding to block on this, apologies.)

Comment 28

[Olli Pettay [:smaug][[e-mail]]]

It wasn't answered and I commented my worry about this in
https://phabricator.services.mozilla.com/D79495
(but r+'ed anyhow)

Comment 29

[Simon Giesecke [:sg] [he/him]]

smaug, while I somehow missed your question saw your comment on the patch, and I planned on responding to that, I somehow missed your comment here.

I am not sure: From my point of view, it is desirable to reduce custom (for/while) loops and in particular use range-based for because it is more expressive. While a custom (for/while) loop has several places where the iteration can be controlled, the range-based for syntax restricts this to the range clause. It is clear that the loop body has only access to the current item, and cannot modify the iterator. (I am not sure if I can add anything to the general reasons why range-based was introduced to the C++ standard).

I am not completely sure how this relates to your comment on the patch, and I would like to better understand that. In what sense does it hide how the iteration happens? Can you point me to some example where using range-based for caused a security issue?

Comment 30

[Olli Pettay [:smaug][[e-mail]]]

I don't expect there to be security issues when dealing with nsTObserverArray, since that is designed to deal with mutations. But we've had
several security critical issues when some other data structure has been iterated using ranged-for and while iterating, something has modified the data structure. The issue there is that range-for hides what actually happens underneath, and in some sense it is too
handy way to iterate, so the patch author, nor the reviewer might not think about what happens if the data structure is modified.
(nsTArray should just crash safely these days, but even a safe crash is rather annoying for the user.)

Comment 31

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6a67e37ca0cc
Use range-based for with nsTObserverArray in dom/debugger. r=loganfsmyth
https://hg.mozilla.org/integration/autoland/rev/eff64e6c4ac9
Use range-based for with nsTObserverArray in layout/style. r=heycam

Comment 32

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/6a67e37ca0cc
https://hg.mozilla.org/mozilla-central/rev/eff64e6c4ac9

Comment 33

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8dd1af6f4953
Use range-based for with nsTObserverArray in dom/base. r=asuth,dom-workers-and-storage-reviewers

Comment 34

[Narcis Beleuzu [:NarcisB]]

Backed out for bustages on nsGlobalWindowInner.cpp

Backout link: https://hg.mozilla.org/integration/autoland/rev/324d5257f6f7cd287565dd822b13d3b5acc0ef14
Log link: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=307515741&repo=autoland&lineNumber=25276

Comment 35

[Simon Giesecke [:sg] [he/him]]

(In reply to Narcis Beleuzu [:NarcisB] from comment #34)

Backed out for bustages on nsGlobalWindowInner.cpp

Backout link: https://hg.mozilla.org/integration/autoland/rev/324d5257f6f7cd287565dd822b13d3b5acc0ef14
Log link: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=307515741&repo=autoland&lineNumber=25276

Sorry for that, I resolved the bustage and updated the patch, and will re-land it.

Comment 36

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6e112475566e
Use range-based for with nsTObserverArray in dom/base. r=asuth,dom-workers-and-storage-reviewers

Comment 37

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/6e112475566e