1372994 - Crash in mozalloc_abort | abort | encoding_c::encoding_for_name

Status: RESOLVED FIXED

(Core :: Internationalization, defect)

Description

[Marcia Knous [:marcia]]

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.

Comment 1

[Henri Sivonen (: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.

Comment 2

[Andrew McCreight [:mccr8]]

It looks like the Linux signature for this is [@ firefox-bin@0x5b17 ].

Comment 3

[Andrew McCreight [:mccr8]]

It looks like the Windows signature may be [@ encoding_c::encoding_for_name ]:
  bp-df75fcab-d1ef-4493-8a74-18e2f0170614

Comment 4

[Henri Sivonen (:hsivonen)]

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.

Comment 5

[Henri Sivonen (:hsivonen)]

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 6

[Henri Sivonen (:hsivonen)]

Attachment: Bug 1372994 - Clear up leftover ISO-8859-1 encoding names.

Review commit: https://reviewboard.mozilla.org/r/149322/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/149322/

Comment 7

[Henri Sivonen (:hsivonen)]

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 8

[Henri Sivonen (:hsivonen)]

Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1946fe465a52

Comment 9

[Henri Sivonen (:hsivonen)]

Attachment: Bug 1372994 addendum - Sanitize encoding names from old cache entries.

Review commit: https://reviewboard.mozilla.org/r/149324/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/149324/

Comment 10

[Masatoshi Kimura [:emk]]

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).

Comment 11

[Masatoshi Kimura [:emk]]

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.

Comment 12

[Henri Sivonen (:hsivonen)]

Comment on attachment 8877924 [details]
Bug 1372994 - Clear up leftover ISO-8859-1 encoding names.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/149322/diff/1-2/

Comment 13

[Henri Sivonen (:hsivonen)]

Comment on attachment 8877925 [details]
Bug 1372994 addendum - Sanitize encoding names from old cache entries.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/149324/diff/1-2/

Comment 14

[Henri Sivonen (:hsivonen)]

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 15

[Henri Sivonen (:hsivonen)]

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

[Pulsebot]

Pushed by [e-mail]:
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

[Andrew McCreight [:mccr8]]

[@ 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

Comment 18

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/d6334ae880c8
https://hg.mozilla.org/mozilla-central/rev/8914e8848f13

Comment 19

[Julian Seward [:jseward]]

This is #2 topcrash in the Windows nightly of 20170615030208.
Presumably it will disappear once the fix makes it into the nightly channel.

Comment 21

[Henri Sivonen (:hsivonen)]

(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

[Andrew McCreight [:mccr8]]

(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.