Last Comment Bug 718500 - View > Character Encoding > Auto-Detect menupopup is broken
: View > Character Encoding > Auto-Detect menupopup is broken
Status: RESOLVED FIXED
: regression
Product: Core
Classification: Components
Component: Internationalization (show other bugs)
: 12 Branch
: x86 Windows 7
: -- normal (vote)
: mozilla12
Assigned To: Makoto Kato [:m_kato]
:
Mentors:
Depends on:
Blocks: 713519
  Show dependency treegraph
 
Reported: 2012-01-16 12:35 PST by Alice0775 White
Modified: 2012-02-09 16:05 PST (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
-


Attachments
fix (3.40 KB, patch)
2012-01-16 23:35 PST, Makoto Kato [:m_kato]
no flags Details | Diff | Splinter Review
fix v2 (7.06 KB, patch)
2012-01-17 03:15 PST, Makoto Kato [:m_kato]
smontagu: review+
Details | Diff | Splinter Review

Description Alice0775 White 2012-01-16 12:35:14 PST
Build Identifier:
http://hg.mozilla.org/mozilla-central/rev/047c8ba7d2e4
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:12.0a1) Gecko/20120116 Firefox/12.0a1 ID:20120116031100

View > Character Encoding > Auto-Detect menupopup is broken

Reproducible: Always

Steps to Reproduce:
1. Open Firefox with clean profile and Google Earth Plugin
2. Open http://maps.google.com/
3. Alt > View > Character Encoding > Auto-Detect


Actual Results:
  No menu pops up

Expected Results:
  Auto-Detect menu should pop up

Regression window(m-c)
Works:
http://hg.mozilla.org/mozilla-central/rev/964b118ac852
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:12.0a1) Gecko/20120113 Firefox/12.0a1 ID:20120113031050
Fails:
http://hg.mozilla.org/mozilla-central/rev/8d4638feec54
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:12.0a1) Gecko/20120113 Firefox/12.0a1 ID:20120113005738
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=964b118ac852&tochange=8d4638feec54


Regression window(m-i)
Works:
http://hg.mozilla.org/integration/mozilla-inbound/rev/3c371703da3c
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:12.0a1) Gecko/20120112 Firefox/12.0a1 ID:20120112080524
Fails:
http://hg.mozilla.org/integration/mozilla-inbound/rev/d7abb1f2efc8
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:12.0a1) Gecko/20120112 Firefox/12.0a1 ID:20120112084225
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=3c371703da3c&tochange=d7abb1f2efc8

Suspected:
d76bf407d5c1	Asaf Romano — Bug 713519 - getCharsetAlias should throw for unknown character sets. r=smontagu.
Comment 1 Alice0775 White 2012-01-16 12:36:11 PST
Err,
>1. Open Firefox with clean profile and Google Earth Plugin
1. Open Firefox with clean profile
Comment 2 Makoto Kato [:m_kato] 2012-01-16 21:21:31 PST
nsCharsetConverterManager::GetList should add item even if getCharsetAlias with "chardet." prefix is failed.
Comment 3 Makoto Kato [:m_kato] 2012-01-16 23:35:41 PST
Created attachment 589111 [details] [diff] [review]
fix
Comment 4 Simon Montagu :smontagu 2012-01-17 00:34:25 PST
Comment on attachment 589111 [details] [diff] [review]
fix

Review of attachment 589111 [details] [diff] [review]:
-----------------------------------------------------------------

::: intl/uconv/src/nsCharsetConverterManager.cpp
@@ -268,5 @@
>      if (NS_FAILED(supStr->GetData(name)))
>        continue;
>  
>      fullName += name;
> -    rv = GetCharsetAlias(fullName.get(), alias);

I wonder do we need the GetCharsetAlias call even in the non-autodetection case? Isn't fullName always going to be a canonical name, since it's returned from the registered encoder/decoder names? Or am I missing something?
Comment 5 Makoto Kato [:m_kato] 2012-01-17 01:00:25 PST
(In reply to Simon Montagu from comment #4)
> Comment on attachment 589111 [details] [diff] [review]
> fix
> 
> Review of attachment 589111 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: intl/uconv/src/nsCharsetConverterManager.cpp
> @@ -268,5 @@
> >      if (NS_FAILED(supStr->GetData(name)))
> >        continue;
> >  
> >      fullName += name;
> > -    rv = GetCharsetAlias(fullName.get(), alias);
> 
> I wonder do we need the GetCharsetAlias call even in the non-autodetection
> case? Isn't fullName always going to be a canonical name, since it's
> returned from the registered encoder/decoder names? Or am I missing
> something?

By bug 206379 (replacing nsIAtom), GetList seems to call GetCharsetAlias.

When I debug again, tt is sure that it is unnecessary to use GetCharsetAlias...
Should we remove GetCharsetAlias calling on GetList?  If fullname is alias, we should fix charset list into category.
Comment 6 Simon Montagu :smontagu 2012-01-17 01:25:18 PST
Yes, I think we should remove the call to GetCharsetAlias. If you want to be extra cautious, you could add a test that all the names returned from GetDecoderList/GetEncoderList are not changed by calling GetCharsetAlias on them.
Comment 7 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2012-01-17 01:31:15 PST
Thanks for taking care of this, Makoto!
Comment 8 Makoto Kato [:m_kato] 2012-01-17 03:15:14 PST
Created attachment 589144 [details] [diff] [review]
fix v2
Comment 9 Makoto Kato [:m_kato] 2012-01-17 04:22:53 PST
Comment on attachment 589144 [details] [diff] [review]
fix v2

same result before landing bug 713519.
Comment 10 :Ms2ger (⌚ UTC+1/+2) 2012-01-17 04:33:16 PST
Comment on attachment 589144 [details] [diff] [review]
fix v2

>@@ -257,28 +256,23 @@ nsCharsetConverterManager::GetList(const
>+    nsCAutoString fullName(aPrefix);
>+    fullName.Append(name);
>+    rv = array->AppendElement(fullName) ? NS_OK : NS_ERROR_OUT_OF_MEMORY;

rv seems unused here.
Comment 11 Simon Montagu :smontagu 2012-01-17 10:07:05 PST
Comment on attachment 589144 [details] [diff] [review]
fix v2

Review of attachment 589144 [details] [diff] [review]:
-----------------------------------------------------------------

::: intl/uconv/src/nsCharsetConverterManager.cpp
@@ +266,5 @@
>        continue;
>  
> +    nsCAutoString fullName(aPrefix);
> +    fullName.Append(name);
> +    rv = array->AppendElement(fullName) ? NS_OK : NS_ERROR_OUT_OF_MEMORY;

Add |NS_ENSURE_SUCCESS(rv, rv)| here and r=me (thanks, Ms2ger!)
Comment 12 :Ms2ger (⌚ UTC+1/+2) 2012-01-17 10:11:31 PST
(Might be better as NS_ENSURE_TRUE(array->AppendElement(fullName), NS_ERROR_OUT_OF_MEMORY) or a boolean temporary)
Comment 14 :Ms2ger (⌚ UTC+1/+2) 2012-01-18 02:50:37 PST
https://hg.mozilla.org/mozilla-central/rev/715e4b7e3b51

Note You need to log in before you can comment on or make changes to this bug.