Closed Bug 1372994 Opened 3 years ago Closed 3 years ago

Crash in mozalloc_abort | abort | encoding_c::encoding_for_name

Categories

(Core :: Internationalization, defect, critical)

56 Branch
Unspecified
macOS
defect
Not set
critical

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)
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)
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 ]
See Also: → 1373070
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 ]
See Also: → 1373045
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.
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 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.
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 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+
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.
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.
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
[@ 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 ]
This is #2 topcrash in the Windows nightly of 20170615030208.
Presumably it will disappear once the fix makes it into the nightly channel.
Duplicate of this bug: 1373369
(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.
(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.