"Assertion failure: !IsReadonly() (shared StringBuffer in SizeOfIncludingThisMustBeUnshared)" with <object id>

RESOLVED FIXED in Firefox 45

Status

()

--
critical
RESOLVED FIXED
3 years ago
10 days ago

People

(Reporter: jruderman, Assigned: njn)

Tracking

(Blocks: 1 bug, {assertion, testcase})

Trunk
mozilla45
assertion, testcase
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox45 fixed)

Details

Attachments

(3 attachments)

(Reporter)

Description

3 years ago
Posted file testcase
1. Load the testcase
2. Quickly click "Measure" in about:memory

Assertion failure: !IsReadonly() (shared StringBuffer in SizeOfIncludingThisMustBeUnshared), at xpcom/string/nsSubstring.cpp:323

This assertion was upgraded to fatal in bug 1225365.
(Reporter)

Comment 1

3 years ago
Posted file stack
Yeah, nsStringHashKey::SizeOfExcludingThis shouldn't be assuming its string is unshared.  It even has a comment about how it shouldn't be assuming it...

Looks like a regression from bug 1046281 and affects some of the other hashkeys too.
Blocks: 1046281
Assignee: nobody → continuation
Honestly, I'm not really sure of the value of ever using SizeOfIncludingThisMustBeUnshared() instead of SizeOfIncludingThisIfUnshared().
What do you think about just ditching MustBeUnshared() for IfUnshared(), Nick?
Flags: needinfo?(n.nethercote)
The former landed in bug 671299, and the latter in bug 712835.
(Assignee)

Comment 6

3 years ago
> What do you think about just ditching MustBeUnshared() for IfUnshared(), Nick?

Yeah, good idea. It's just adding unnecessary brittleness.
Flags: needinfo?(n.nethercote)
(Assignee)

Updated

3 years ago
Assignee: continuation → n.nethercote
(Assignee)

Comment 7

3 years ago
The patch changes all uses of SizeOfIncludingThisMustBeUnshared() to
SizeOfIncludingThisIfUnshared(). This incurs the (tiny) cost of an unnecessary
IsReadonly() check for guaranteed-unshared strings, but avoids the possible
assertion failures that would occur when MustBeUnshared() was used incorrectly
on shared strings, which is an easy mistake to make.
Attachment #8694444 - Flags: review?(continuation)
Comment on attachment 8694444 [details] [diff] [review]
Remove SizeOfIncludingThisMustBeUnshared() from string classes

Review of attachment 8694444 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for fixing this.

::: xpcom/string/nsTSubstring.cpp
@@ +1005,5 @@
>      mozilla::MallocSizeOf aMallocSizeOf) const
>  {
>    if (mFlags & F_SHARED) {
>      return nsStringBuffer::FromData(mData)->
> +           SizeOfIncludingThisIfUnshared(aMallocSizeOf);

This new indentation looks weird to me. Maybe more like the old one?
Attachment #8694444 - Flags: review?(continuation) → review+
(Assignee)

Comment 9

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/d746378ad672eda7c9f33a0e93e08291bff5f5e3
Bug 1229458 - Remove SizeOfIncludingThisMustBeUnshared() from string classes. r=mccr8.

Comment 10

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/d746378ad672
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox45: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
Component: DOM → DOM: Core & HTML
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.