Closed Bug 1462912 Opened 2 years ago Closed 2 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)

Product:
Core
Component:
IPC
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla62
Tracking Flags:
Tracking Status
firefox-esr52 61+ fixed
firefox-esr60 61+ fixed
firefox60 --- unaffected
firefox61 + fixed
firefox62 blocking verified

People

(Reporter: ebalazs_, Assigned: Alex_Gaynor)

References

Details

(Keywords: regression, Whiteboard: [stockwell fixed:product])

Attachments

(1 file)

Bug 1462912 - fixed BufferList::Extract to handle the case where the call consumes the entirety of the BufferList;
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
:aryx, just so I understand, this patch is all green on central, but on -beta merge fails?

Will take a look, thanks.
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 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)
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 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 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
Assignee: nobody → agaynor
Flags: needinfo?(agaynor)
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+
Keywords: checkin-needed
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
https://hg.mozilla.org/mozilla-central/rev/96a6ea5ea346
Status: NEW → RESOLVED
Closed: 2 years ago
status-firefox62: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
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?
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+
https://hg.mozilla.org/releases/mozilla-beta/rev/cef7f526b60e
status-firefox61: affected → fixed
Flags: in-testsuite+
Whiteboard: [stockwell disable-recommended] → [stockwell fixed:product]
You need to log in before you can comment on or make changes to this bug.