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

defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla62
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)

[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.
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: 5 years ago
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
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.