1462912 - permafail many crashes [@ mozilla::BufferList<InfallibleAllocPolicy>::Extract(mozilla::BufferList<InfallibleAllocPolicy>::IterImpl &,unsigned int,bool *)] on opt when Gecko 62 merges to Beta on 2018-06-14

Status: VERIFIED FIXED

(Core :: IPC, defect)

Description

[Eliza Balazs [:ebalazs_]]

[Tracking Requested - why for this release]:

Central as Beta: 

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9ce35adca3e63950635f1da3e810e6d994292c24&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable&selectedJob=179336202

TEST-UNEXPECTED-FAIL | devtools/client/storage/test/browser_storage_basic.js | Test timed out -
TEST-UNEXPECTED-TIMEOUT | devtools/client/storage/test/browser_storage_basic.js | application timed out after 370 seconds with no output
Force-terminating active process(es).
PROCESS-CRASH | Main app process exited normally | application crashed [None]
PROCESS-CRASH | Main app process exited normally | application crashed [@ libc-2.23.so + 0xfb74d]
PROCESS-CRASH | Main app process exited normally | application crashed [@ mozilla::BufferList<InfallibleAllocPolicy>::Extract]

This is a regression from bug 1451991

Comment 1

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

Alex, today's beta simulations have mass failures with application crashed [@ mozilla::BufferList<InfallibleAllocPolicy>::Extract(mozilla::BufferList<InfallibleAllocPolicy>::IterImpl &,unsigned int,bool *)] https://treeherder.mozilla.org/#/jobs?repo=try&revision=9ce35adca3e63950635f1da3e810e6d994292c24&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable&group_state=expanded&selectedJob=179340771

For this bug, a test fails first but the crash signature also has this one https://treeherder.mozilla.org/logviewer.html#?job_id=179335969&repo=try

Please take a look

Comment 2

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

Issue goes away if bug 1456189 gets reverted: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7c9396b1a82c9ea08b9a6dd45b06502831828acc&selectedJob=179354616

Comment 3

[Alex Gaynor [:Alex_Gaynor]]

:aryx, just so I understand, this patch is all green on central, but on -beta merge fails?

Will take a look, thanks.

Comment 4

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

Alex: Yes, this hits only beta, see this beta push as example: https://treeherder.mozilla.org/#/jobs?repo=try&revision=516fb3a0a5aed0a10833dfe3dc585d4872b3be15&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable

Import the patches listed there if you want to do your own pushes to try (the backout at the bottom is required to get Windows builds working until bug 1461304 is fixed).

Comment 5

[Intermittent Failures Robot]

321 failures in 86 pushes (3.733 failures/push) were associated with this bug yesterday.    

** This test has failed more than 150 times in the last 21 days. It should be disabled until it can be fixed. ** 

Repository breakdown:
* try: 321

Platform breakdown:
* windows10-64: 70
* windows10-64-nightly: 57
* windows10-64-devedition: 41
* windows7-32-nightly: 34
* windows7-32: 29
* windows7-32-devedition: 28
* linux64: 13
* windows10-64-qr: 10
* linux64-nightly: 7
* osx-10-10: 5
* linux32-devedition: 5
* macosx64-nightly: 4
* linux64-devedition: 4
* linux32-nightly: 4
* linux32: 4
* macosx64-devedition: 3
* linux64-qr: 3

For more details, see:
https://treeherder.mozilla.org/intermittent-failures.html#/bugdetails?bug=1462912&startday=2018-05-21&endday=2018-05-21&tree=trunk

Comment 6

[Alex Gaynor [:Alex_Gaynor]]

Attachment: Bug 1462912 - fixed BufferList::Extract to handle the case where the call consumes the entirety of the BufferList;

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

Comment 7

[Nathan Froyd [:froydnj]]

Comment on attachment 8979658 [details]
Bug 1462912 - fixed BufferList::Extract to handle the case where the call consumes the entirety of the BufferList;

https://reviewboard.mozilla.org/r/245804/#review251900

Thanks for working on this.  I am unconvinced of the correctness of the assertion, but I could totally be missing something!

::: mfbt/BufferList.h:618
(Diff revision 1)
> +      (aIter.mSegment == copyStart + segmentsToCopy) ||
> +      (aIter.Done() && aIter.mSegment == copyStart + segmentsToCopy - 1));

Just so I understand this: this assert is saying something like: "either we copied all the segments over or we finished the iterator and therefore copied fewer segments"?  That seems like a really peculiar assertion to me.  In particular, it feels like that means that we did something wrong in computing `segmentsToCopy` in the first place.

::: mfbt/BufferList.h:620
(Diff revision 1)
>      mSegments.erase(mSegments.begin() + copyStart,
>                      mSegments.begin() + copyStart + segmentsToCopy);

This code, given the above assert, also makes me nervous, because we could be erasing more than what we thought we copied?

Comment 8

[Alex Gaynor [:Alex_Gaynor]]

Comment on attachment 8979658 [details]
Bug 1462912 - fixed BufferList::Extract to handle the case where the call consumes the entirety of the BufferList;

https://reviewboard.mozilla.org/r/245804/#review251900

> Just so I understand this: this assert is saying something like: "either we copied all the segments over or we finished the iterator and therefore copied fewer segments"?  That seems like a really peculiar assertion to me.  In particular, it feels like that means that we did something wrong in computing `segmentsToCopy` in the first place.

The reason for this is that `Advance` does not increment `mSegment` if you're at the very end: https://searchfox.org/mozilla-central/source/mfbt/BufferList.h#242

If you're at the very end of the bufferlist, it leaves you there, otherwise it puts you at the start of the next segment. I can't think of a better way to express this invariant with the way the iterator works at the moment; would a comment explaining this help?

Comment 9

[Nathan Froyd [:froydnj]]

Comment on attachment 8979658 [details]
Bug 1462912 - fixed BufferList::Extract to handle the case where the call consumes the entirety of the BufferList;

https://reviewboard.mozilla.org/r/245804/#review251900

> The reason for this is that `Advance` does not increment `mSegment` if you're at the very end: https://searchfox.org/mozilla-central/source/mfbt/BufferList.h#242
> 
> If you're at the very end of the bufferlist, it leaves you there, otherwise it puts you at the start of the next segment. I can't think of a better way to express this invariant with the way the iterator works at the moment; would a comment explaining this help?

Ugh, that feels like a very non-intuitive way for the iterator to work.  If we're not going to fix the iterator here (which is probably the correct thing to get beta uplift simulations unblocked?), I think it's definitely worth a comment about this behavior.  Can you file a followup bug on making the iterator work like an iterator is supposed to work?

Comment 10

[Alex Gaynor [:Alex_Gaynor]]

Comment on attachment 8979658 [details]
Bug 1462912 - fixed BufferList::Extract to handle the case where the call consumes the entirety of the BufferList;

https://reviewboard.mozilla.org/r/245804/#review251900

> Ugh, that feels like a very non-intuitive way for the iterator to work.  If we're not going to fix the iterator here (which is probably the correct thing to get beta uplift simulations unblocked?), I think it's definitely worth a comment about this behavior.  Can you file a followup bug on making the iterator work like an iterator is supposed to work?

bug 1463516

Comment 11

[Alex Gaynor [:Alex_Gaynor]]

Comment on attachment 8979658 [details]
Bug 1462912 - fixed BufferList::Extract to handle the case where the call consumes the entirety of the BufferList;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/245804/diff/1-2/

Comment 12

[Nathan Froyd [:froydnj]]

Comment on attachment 8979658 [details]
Bug 1462912 - fixed BufferList::Extract to handle the case where the call consumes the entirety of the BufferList;

https://reviewboard.mozilla.org/r/245804/#review251930

Thank you for clearing things up for me!

::: mfbt/BufferList.h:617
(Diff revisions 1 - 2)
>          Segment(mSegments[aIter.mSegment].mData,
>                  mSegments[aIter.mSegment].mSize,
>                  mSegments[aIter.mSegment].mCapacity));
>        aIter.Advance(*this, aIter.RemainingInSegment());
>      }
> +    // Due to the way IterImpl works, there are two case here: (1) if we've

Nit: "two cases"

Comment 13

[Alex Gaynor [:Alex_Gaynor]]

Comment on attachment 8979658 [details]
Bug 1462912 - fixed BufferList::Extract to handle the case where the call consumes the entirety of the BufferList;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/245804/diff/2-3/

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/96a6ea5ea346
fixed BufferList::Extract to handle the case where the call consumes the entirety of the BufferList; r=froydnj

Comment 15

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/96a6ea5ea346

Comment 16

[Alex Gaynor [:Alex_Gaynor]]

Comment on attachment 8979658 [details]
Bug 1462912 - fixed BufferList::Extract to handle the case where the call consumes the entirety of the BufferList;

Uplift needed to support uplifting of bug 1456189, see the request there for details.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8979658 [details]
Bug 1462912 - fixed BufferList::Extract to handle the case where the call consumes the entirety of the BufferList;

Approved for 61.0b8 and ESR 60.1.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/cef7f526b60e

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr60/rev/c3fadedf7d3d

Comment 20

[Intermittent Failures Robot]

327 failures in 632 pushes (0.517 failures/push) were associated with this bug in the last 7 days. 

This is the #6 most frequent failure this week. 

** This failure happened more than 75 times this week! Resolving this bug is a very high priority. **

** Try to resolve this bug as soon as possible. If unresolved for 1 week, the affected test(s) may be disabled. **   

Repository breakdown:
* try: 327

Platform breakdown:
* windows10-64: 64
* windows10-64-nightly: 56
* windows10-64-devedition: 40
* windows7-32: 35
* windows7-32-devedition: 34
* windows7-32-nightly: 29
* linux64: 16
* windows10-64-qr: 10
* osx-10-10: 6
* linux32-nightly: 6
* linux64-qr: 5
* linux64-nightly: 5
* linux32-devedition: 5
* macosx64-nightly: 4
* macosx64-devedition: 4
* linux64-devedition: 4
* linux32: 4

For more details, see:
https://treeherder.mozilla.org/intermittent-failures.html#/bugdetails?bug=1462912&startday=2018-05-21&endday=2018-05-27&tree=trunk

Comment 21

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

Verified fixed in today's beta simulation: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9b1d7e4d54942c4ec9c01d7cea0c24f7b8876e30&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/f18535a212da