Crash in mozilla::detail::nsStringRepr::First (called from nsCommandLine::HandleFlagWithParam)

RESOLVED FIXED in Firefox 54

Status

()

defect
P2
critical
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: mats, Assigned: dbaron)

Tracking

({crash, regression})

55 Branch
mozilla55
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox-esr45 unaffected, thunderbird_esr45 unaffected, thunderbird_esr52 unaffected, firefox-esr52 unaffected, firefox53 unaffected, firefox54+ fixed, firefox55+ fixed)

Details

(crash signature)

Attachments

(1 attachment)

Reporter

Description

2 years ago
This bug was filed from the Socorro interface and is 
report bp-547a1907-53b2-4bb0-a271-d8ba10170508.
=============================================================

This is a new crash in v55 due to:
MOZ_CRASH Reason:  MOZ_RELEASE_ASSERT(mLength > 0) (|First()| called on an empty string)

The stack is the same for all reports so far; the call comes from:
nsCommandLine::HandleFlagWithParam
Hi :mats,
I'm not sure why you mark 54 affected because I didn't see any crashes in 54 for the past 14 days.
Flags: needinfo?(mats)
Reporter

Comment 2

2 years ago
I didn't see any crashes in v54 either; however, bug 1340577 which added this runtime assertion
is marked as fixed in v54.
Flags: needinfo?(mats)

Updated

2 years ago
Component: String → Startup and Profile System
Product: Core → Toolkit
I think it's just showing up at a different (and less comprehensible) stack in 54; see bug 1362989.
Assignee: nobody → dbaron
Status: NEW → ASSIGNED
I think this is an important crash to fix because it seems to have the risk of creating a Firefox that crashes on startup all the time -- for whatever setup users have that causes arguments like this.
Tracking for 54/55 so we don't ship with this crash.

Comment 8

2 years ago
Comment on attachment 8866110 [details] [diff] [review]
Don't call First() on empty strings in nsCommandLine::HandleFlagWithParam

Can this be a const reference instead of a pointer? It seems unconventional in our code to be using nsString* like this.

r=me with that change or please come back if a pointer is in fact preferable
Attachment #8866110 - Flags: review?(benjamin) → review+
The reason I preferred a pointer is so that I could null it out where it becomes invalid.  Maybe there should be a comment next to the nulling out saying "// becomes invalid after RemoveArguments"?
Flags: needinfo?(benjamin)
Reporter

Comment 10

2 years ago
Use a scope?
Yeah, I guess that works.  There was some reason I didn't want to do that yesterday, but I've forgotten.
Flags: needinfo?(benjamin)
Comment on attachment 8866110 [details] [diff] [review]
Don't call First() on empty strings in nsCommandLine::HandleFlagWithParam

Approval Request Comment
[Feature/Bug causing the regression]:bug 1340577
[User impact if declined]:crashes, probably specific to a small portion of users, but pretty repeatable (might cause them to give up)
[Is this code covered by automated tests?]: not aware of any
[Has the fix been verified in Nightly?]: no
[Needs manual test from QE? If yes, steps to reproduce]: Probably not, given that the fix is obvious.
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: No.
[Why is the change risky/not risky?]: It's adding an IsEmpty() test on the string before calling First()
[String changes made/needed]: no
Attachment #8866110 - Flags: approval-mozilla-beta?

Comment 14

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/63daa5635269
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Comment on attachment 8866110 [details] [diff] [review]
Don't call First() on empty strings in nsCommandLine::HandleFlagWithParam

Fix a crash. Beta54+. Should be in 54 beta 8.
Attachment #8866110 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
I filed bug 1364982 (with pull request) to improve the crash signature here.
(In reply to David Baron :dbaron: ⌚️UTC-7 from comment #13)
> [Is this code covered by automated tests?]: not aware of any
> [Has the fix been verified in Nightly?]: no
> [Needs manual test from QE? If yes, steps to reproduce]: Probably not, given
> that the fix is obvious.

Setting qe-verify- based on David's assessment on manual testing needs.
Flags: qe-verify-
Crash Signature: [@ mozilla::detail::nsStringRepr::First] → [@ mozilla::detail::nsStringRepr::First] [@ mozilla::detail::nsStringRepr::First | nsCommandLine::HandleFlagWithParam]
Duplicate of this bug: 1362989
You need to log in before you can comment on or make changes to this bug.