1439525 - [@ InvalidArrayIndex_CRASH | mozilla::wr::ShmSegmentsWriter::Write]

Status: RESOLVED FIXED

(Core :: Graphics: WebRender, defect, P2)

Description

[:gerard-majax]

Report: https://crash-stats.mozilla.com/report/index/3473914e-9b08-4dcd-a1ed-065ae0180220

Hit this when switching to Mastodon pinned tab yesterday (in a container), some time after applying an update. This was my first access to that tab after applying the update.

This is on Linux (Ubuntu 17.10), nightly 20180219100221. I'm hacking some OpenCL-related code at the same time (that is consuming a lot of resources), maybe this has an impact on WebRender?

Comment 1

[Kartikaya Gupta (email:[e-mail])]

This loop looks wrong to me: https://searchfox.org/mozilla-central/rev/0c0ddaa7e859a2b76a56a0e2e9c0de88af166812/gfx/layers/wr/IpcResourceUpdateQueue.cpp#54

I would have expected it be
  for (size_t i = mSmallAllocs.Length() - 1; currAllocLen <= i ; i--) {

nical, can you confirm? You reviewed the last "correction" to this loop in bug 1413651.

At any rate we should add a MOZ_ASSERT(mSmallAllocs.Length() == currAllocLen) after the loop to assert it's doing what it's supposed to.

Comment 2

[Kartikaya Gupta (email:[e-mail])]

Err actually we can't use size_t for that index if we do what I suggested. Maybe better to leave it as-is and use i-1 inside the loop?

Comment 3

[Kartikaya Gupta (email:[e-mail])]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4ef0c772e5622414edd448f66041926d42e8e2cf

Comment 4

[Kartikaya Gupta (email:[e-mail])]

Attachment: Bug 1439525 - Fix freeing of allocated chunks when an allocation fails.

Review commit: https://reviewboard.mozilla.org/r/221716/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/221716/

Comment 5

[Kartikaya Gupta (email:[e-mail])]

Attachment: Bug 1439525 - Remove spurious "continue" statement.

The "continue" statement makes it look like the loop can run multiple
allocations in a row when that never actually happens. (And if it did,
the latter half of the loop would do the wrong thing). This patch
removes the spurious continue statement and replaces it with a more
useful assertion that documents the state at that point.

Review commit: https://reviewboard.mozilla.org/r/221718/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/221718/

Comment 6

[Kartikaya Gupta (email:[e-mail])]

Attachment: Bug 1439525 - Reset the cursor when swapping the shmems out.

Seems like a footgun to leave mCursor pointing to some random value when
the shmems have been swapped out. Currently nobody seems to be reusing
one of these things after a Flush() call but this should make things work
better if anybody tries.

Review commit: https://reviewboard.mozilla.org/r/221720/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/221720/

Comment 7

[Nicolas Silva [:nical]]

Comment on attachment 8952503 [details]
Bug 1439525 - Fix freeing of allocated chunks when an allocation fails.

https://reviewboard.mozilla.org/r/221716/#review228554

oops!

Comment 8

[Nicolas Silva [:nical]]

Comment on attachment 8952504 [details]
Bug 1439525 - Remove spurious "continue" statement.

https://reviewboard.mozilla.org/r/221718/#review228556

Comment 9

[Nicolas Silva [:nical]]

Comment on attachment 8952505 [details]
Bug 1439525 - Reset the cursor when swapping the shmems out.

https://reviewboard.mozilla.org/r/221720/#review228558

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d1241d870e3e
Fix freeing of allocated chunks when an allocation fails. r=nical
https://hg.mozilla.org/integration/autoland/rev/cc82773113d1
Remove spurious "continue" statement. r=nical
https://hg.mozilla.org/integration/autoland/rev/36b8974d32de
Reset the cursor when swapping the shmems out. r=nical

Comment 11

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/d1241d870e3e
https://hg.mozilla.org/mozilla-central/rev/cc82773113d1
https://hg.mozilla.org/mozilla-central/rev/36b8974d32de