Closed
Bug 1372994
Opened 7 years ago
Closed 7 years ago
Crash in mozalloc_abort | abort | encoding_c::encoding_for_name
Categories
(Core :: Internationalization, defect)
Tracking
()
RESOLVED
FIXED
mozilla56
Tracking | Status | |
---|---|---|
firefox-esr52 | --- | unaffected |
firefox54 | --- | unaffected |
firefox55 | --- | unaffected |
firefox56 | --- | fixed |
People
(Reporter: marcia, Assigned: hsivonen)
References
Details
(Keywords: crash, regression)
Crash Data
Attachments
(2 files)
This bug was filed from the Socorro interface and is
report bp-f8bade28-15e9-4dfa-944a-c8b4d0170614.
=============================================================
New crash seen on Mac Desktop and Fennec using 20170614030206: http://bit.ly/2ri9Hw0
Bug 1261841 landed in this timeframe, and touched code which shows up in the stack (third_party/rust/encoding_rs/src/lib.rs:2317). ni on Henri in case that bug is related to the crash.
Flags: needinfo?(hsivonen)
Assignee | ||
Comment 1•7 years ago
|
||
This is an assertion crash arising from a string that's not a canonical name getting passed to an encoding lookup function that must only be given canonical names. I'll take a look to find out how a non-canonical name might find its way here.
Assignee: nobody → hsivonen
Status: NEW → ASSIGNED
Flags: needinfo?(hsivonen)
Comment 2•7 years ago
|
||
It looks like the Linux signature for this is [@ firefox-bin@0x5b17 ].
Crash Signature: [@ mozalloc_abort | abort | encoding_c::encoding_for_name] → [@ mozalloc_abort | abort | encoding_c::encoding_for_name][@ firefox-bin@0x5b17 ]
Updated•7 years ago
|
Comment 3•7 years ago
|
||
It looks like the Windows signature may be [@ encoding_c::encoding_for_name ]:
bp-df75fcab-d1ef-4493-8a74-18e2f0170614
Crash Signature: [@ mozalloc_abort | abort | encoding_c::encoding_for_name][@ firefox-bin@0x5b17 ] → [@ mozalloc_abort | abort | encoding_c::encoding_for_name][@ firefox-bin@0x5b17 ] [@ encoding_c::encoding_for_name ]
Assignee | ||
Updated•7 years ago
|
Assignee | ||
Comment 4•7 years ago
|
||
Bug 1373045 has a test case that points in the right direction. So far, it looks like the assertion here unmasked a pre-existing bug.
Assignee | ||
Comment 5•7 years ago
|
||
One problem is that we treat our cache as a trusted source of names, but the names changed, so a pre-existing cache can contain now-bogus names.
Comment hidden (mozreview-request) |
Assignee | ||
Comment 7•7 years ago
|
||
mozreview-review |
Comment on attachment 8877924 [details]
Bug 1372994 - Clear up leftover ISO-8859-1 encoding names.
https://reviewboard.mozilla.org/r/149322/#review153834
::: dom/jsurl/nsJSProtocolHandler.cpp:295
(Diff revision 1)
> return NS_ERROR_OUT_OF_MEMORY;
> }
>
> char *bytes;
> uint32_t bytesLen;
> - NS_NAMED_LITERAL_CSTRING(isoCharset, "ISO-8859-1");
> + NS_NAMED_LITERAL_CSTRING(isoCharset, "windows-1252");
This may be the most questionable-looking bit in this patch. This should not created any _new_ breakage, though, since the old ISO-8859-1 decoder actually had the windows-1252 behavior.
Assignee | ||
Comment 8•7 years ago
|
||
Comment hidden (mozreview-request) |
Comment 10•7 years ago
|
||
mozreview-review |
Comment on attachment 8877924 [details]
Bug 1372994 - Clear up leftover ISO-8859-1 encoding names.
https://reviewboard.mozilla.org/r/149322/#review153922
::: toolkit/components/search/nsSearchService.js:116
(Diff revision 1)
> // cause big delays when loading them at startup.
> const MAX_ICON_SIZE = 10000;
>
> -// Default charset to use for sending search parameters. ISO-8859-1 is used to
> +// Default charset to use for sending search parameters. windows-1252 is used to
> // match previous nsInternetSearchService behavior.
> -const DEFAULT_QUERY_CHARSET = "ISO-8859-1";
> +const DEFAULT_QUERY_CHARSET = "windows-1252";
Since nsTextToSubURI::ConvertAndEscape and nsTextToSubURI::UnEscapeAndConvert takes a label rather than a name, this change is unneccessary.
Moreover, this is a breaking change because the generated search URL will contain this value. Please revert this change (and add a comment why the label is used here).
Attachment #8877924 -
Flags: review?(VYV03354) → review+
Comment 11•7 years ago
|
||
mozreview-review |
Comment on attachment 8877925 [details]
Bug 1372994 addendum - Sanitize encoding names from old cache entries.
https://reviewboard.mozilla.org/r/149324/#review153926
::: dom/html/nsHTMLDocument.cpp:360
(Diff revision 1)
> return;
> }
>
> nsCString cachedCharset;
> rv = aCachingChannel->GetCacheTokenCachedCharset(cachedCharset);
> + if (cachedCharset.IsEmpty() || NS_FAILED(rv)) {
Why the evaluation order is swapped? (old: rv -> cachedCharset, new: cachedCharset -> rv) You should not touch cachedCharset if GetCacheTokenCachedCharset failed.
Attachment #8877925 -
Flags: review?(VYV03354) → review+
Updated•7 years ago
|
status-firefox54:
--- → unaffected
status-firefox-esr52:
--- → unaffected
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 14•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8877925 [details]
Bug 1372994 addendum - Sanitize encoding names from old cache entries.
https://reviewboard.mozilla.org/r/149324/#review153926
> Why the evaluation order is swapped? (old: rv -> cachedCharset, new: cachedCharset -> rv) You should not touch cachedCharset if GetCacheTokenCachedCharset failed.
Swapped order back.
Assignee | ||
Comment 15•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8877924 [details]
Bug 1372994 - Clear up leftover ISO-8859-1 encoding names.
https://reviewboard.mozilla.org/r/149322/#review153922
> Since nsTextToSubURI::ConvertAndEscape and nsTextToSubURI::UnEscapeAndConvert takes a label rather than a name, this change is unneccessary.
>
> Moreover, this is a breaking change because the generated search URL will contain this value. Please revert this change (and add a comment why the label is used here).
Reverted with comment.
Comment 16•7 years ago
|
||
Pushed by hsivonen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d6334ae880c8
Clear up leftover ISO-8859-1 encoding names. r=emk
https://hg.mozilla.org/integration/autoland/rev/8914e8848f13
addendum - Sanitize encoding names from old cache entries. r=emk
Comment 17•7 years ago
|
||
[@ alloc::oom::default_oom_handler ] looks like another variant of this crash on Windows (at least, it is the most common source of this signature).
For instance: bp-6c395eff-54f7-4937-bad9-b43ce0170615
Crash Signature: [@ mozalloc_abort | abort | encoding_c::encoding_for_name][@ firefox-bin@0x5b17 ] [@ encoding_c::encoding_for_name ] → [@ mozalloc_abort | abort | encoding_c::encoding_for_name][@ firefox-bin@0x5b17 ] [@ encoding_c::encoding_for_name ] [@ alloc::oom::default_oom_handler ]
Comment 18•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/d6334ae880c8
https://hg.mozilla.org/mozilla-central/rev/8914e8848f13
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Comment 19•7 years ago
|
||
This is #2 topcrash in the Windows nightly of 20170615030208.
Presumably it will disappear once the fix makes it into the nightly channel.
Assignee | ||
Comment 21•7 years ago
|
||
(In reply to Andrew McCreight [:mccr8] from comment #17)
> [@ alloc::oom::default_oom_handler ] looks like another variant of this
> crash on Windows (at least, it is the most common source of this signature).
It seems that the above annotation now associates WebRender crashes with this bug.
Comment 22•7 years ago
|
||
(In reply to Henri Sivonen (:hsivonen) from comment #21)
> It seems that the above annotation now associates WebRender crashes with
> this bug.
Yes, that will be an issue until bug 1373272 starts getting applied to new crashes. Until then, you'll have to do a search for the signature that facets on proto signatures.
You need to log in
before you can comment on or make changes to this bug.
Description
•