Closed Bug 1363262 Opened 7 years ago Closed 7 years ago

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

Categories

(Toolkit :: Startup and Profile System, defect, P2)

55 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox-esr45 --- unaffected
thunderbird_esr45 --- unaffected
thunderbird_esr52 --- unaffected
firefox-esr52 --- unaffected
firefox53 --- unaffected
firefox54 + fixed
firefox55 + fixed

People

(Reporter: MatsPalmgren_bugz, Assigned: dbaron)

References

Details

(Keywords: crash, regression)

Crash Data

Attachments

(1 file)

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)
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)
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 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)
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?
https://hg.mozilla.org/mozilla-central/rev/63daa5635269
Status: ASSIGNED → RESOLVED
Closed: 7 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]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: