Closed
Bug 1462912
Opened 3 years ago
Closed 3 years ago
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
Categories
(Core :: IPC, defect)
Core
IPC
Tracking
()
VERIFIED
FIXED
mozilla62
People
(Reporter: ebalazs_, Assigned: Alex_Gaynor)
References
Details
(Keywords: regression, Whiteboard: [stockwell fixed:product])
Attachments
(1 file)
59 bytes,
text/x-review-board-request
|
froydnj
:
review+
RyanVM
:
approval-mozilla-beta+
RyanVM
:
approval-mozilla-esr60+
|
Details |
[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
Flags: needinfo?(nchevobbe)
Summary: Bustage eg: devtools/client/storage/test/browser_storage_basic.js → devtools/client/storage/test/browser_storage_basic.js when Gecko 62 merges to Beta on 2018-06-14
Summary: devtools/client/storage/test/browser_storage_basic.js when Gecko 62 merges to Beta on 2018-06-14 → permafail of devtools/client/storage/test/browser_storage_basic.js on opt when Gecko 62 merges to Beta on 2018-06-14
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
Flags: needinfo?(agaynor)
Summary: permafail of devtools/client/storage/test/browser_storage_basic.js on opt when Gecko 62 merges to Beta on 2018-06-14 → permafail of devtools/client/storage/test/browser_storage_basic.js and [@ mozilla::BufferList<InfallibleAllocPolicy>::Extract(mozilla::BufferList<InfallibleAllocPolicy>::IterImpl &,unsigned int,bool *)] on opt when Gecko 62 merges to Beta on 2018-06-14
Issue goes away if bug 1456189 gets reverted: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7c9396b1a82c9ea08b9a6dd45b06502831828acc&selectedJob=179354616
Component: Developer Tools: Storage Inspector → IPC
Flags: needinfo?(nchevobbe)
Product: Firefox → Core
Summary: permafail of devtools/client/storage/test/browser_storage_basic.js and [@ mozilla::BufferList<InfallibleAllocPolicy>::Extract(mozilla::BufferList<InfallibleAllocPolicy>::IterImpl &,unsigned int,bool *)] on opt when Gecko 62 merges to Beta on 2018-06-14 → 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
Assignee | ||
Comment 3•3 years ago
|
||
:aryx, just so I understand, this patch is all green on central, but on -beta merge fails? Will take a look, thanks.
Updated•3 years ago
|
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 hidden (Intermittent Failures Robot) |
Updated•3 years ago
|
Keywords: regression
Comment hidden (mozreview-request) |
![]() |
||
Comment 7•3 years ago
|
||
mozreview-review |
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?
Attachment #8979658 -
Flags: review?(nfroyd)
Assignee | ||
Comment 8•3 years ago
|
||
mozreview-review-reply |
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•3 years ago
|
||
mozreview-review-reply |
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?
Assignee | ||
Comment 10•3 years ago
|
||
mozreview-review-reply |
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 hidden (mozreview-request) |
Assignee | ||
Updated•3 years ago
|
Assignee: nobody → agaynor
Flags: needinfo?(agaynor)
![]() |
||
Comment 12•3 years ago
|
||
mozreview-review |
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"
Attachment #8979658 -
Flags: review?(nfroyd) → review+
Comment hidden (mozreview-request) |
Assignee | ||
Updated•3 years ago
|
Keywords: checkin-needed
Comment 14•3 years ago
|
||
Pushed by nfroyd@mozilla.com: 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
Keywords: checkin-needed
Comment 15•3 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/96a6ea5ea346
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
Assignee | ||
Comment 16•3 years ago
|
||
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.
Attachment #8979658 -
Flags: approval-mozilla-esr60?
Attachment #8979658 -
Flags: approval-mozilla-beta?
Updated•3 years ago
|
tracking-firefox61:
--- → +
tracking-firefox-esr60:
--- → 61+
Comment 17•3 years ago
|
||
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.
Attachment #8979658 -
Flags: approval-mozilla-esr60?
Attachment #8979658 -
Flags: approval-mozilla-esr60+
Attachment #8979658 -
Flags: approval-mozilla-beta?
Attachment #8979658 -
Flags: approval-mozilla-beta+
Updated•3 years ago
|
Comment 18•3 years ago
|
||
bugherderuplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/cef7f526b60e
Flags: in-testsuite+
Whiteboard: [stockwell disable-recommended] → [stockwell fixed:product]
Comment 19•3 years ago
|
||
bugherderuplift |
https://hg.mozilla.org/releases/mozilla-esr60/rev/c3fadedf7d3d
Comment hidden (Intermittent Failures Robot) |
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
Status: RESOLVED → VERIFIED
Comment 22•3 years ago
|
||
uplift |
https://hg.mozilla.org/releases/mozilla-esr52/rev/f18535a212da
You need to log in
before you can comment on or make changes to this bug.
Description
•