593046 - nsJapaneseToUnicode uses the pref service off of the main thread

Status: RESOLVED FIXED

(Core :: Internationalization, defect)

Description

[Shawn Wilsher :sdwilsh]

REFTEST TEST-START | file:///home/cltbld/talos-slave/tryserver_fedora-debug_test-crashtest/build/reftest/tests/intl/uconv/crashtests/563618.html
++DOMWINDOW == 49 (0x9514d10) [serial = 869] [outer = 0x8bb91a0]
###!!! ASSERTION: Cannot be used on a background thread!: 'Error', file /builds/slave/tryserver-linux-debug/build/modules/libpref/src/nsPrefService.cpp, line 871
nsPrefService::CheckAndLogBackgroundThreadUse [modules/libpref/src/nsPrefService.cpp:873]
nsPrefBranch::GetCharPref [modules/libpref/src/nsPrefBranch.cpp:219]
nsPrefService::GetCharPref [modules/libpref/src/nsPrefService.h:60]
nsJapaneseToUnicode::setMapMode [intl/uconv/ucvja/nsJapaneseToUnicode.cpp:64]
nsEUCJPToUnicodeV2::nsEUCJPToUnicodeV2 [intl/uconv/ucvja/nsJapaneseToUnicode.h:96]
nsEUCJPToUnicodeV2Constructor [intl/uconv/src/nsUConvModule.cpp:437]
mozilla::GenericFactory::CreateInstance [GenericFactory.cpp:49]
nsComponentManagerImpl::CreateInstanceByContractID [xpcom/components/nsComponentManager.cpp:1284]
CallCreateInstance [nsComponentManagerUtils.cpp:170]
nsCreateInstanceByContractID::operator() [nsComponentManagerUtils.cpp:210]
nsCOMPtr<nsIUnicodeDecoder>::assign_from_helper [nsCOMPtr.h:1272]
nsCOMPtr<nsIUnicodeDecoder>::operator= [nsCOMPtr.h:731]
nsCharsetConverterManager::GetUnicodeDecoderRaw [intl/uconv/src/nsCharsetConverterManager.cpp:233]
nsHtml5MetaScanner::tryCharset [parser/html/nsHtml5MetaScannerCppSupplement.h:125]
nsHtml5MetaScanner::tryCharset [parser/html/nsHtml5MetaScanner.cpp:699]
nsHtml5MetaScanner::stateLoop [parser/html/nsHtml5MetaScanner.cpp:327]
nsHtml5MetaScanner::sniff [parser/html/nsHtml5MetaScannerCppSupplement.h:69]
nsHtml5StreamParser::SniffStreamBytes [parser/html/nsHtml5StreamParser.cpp:437]
nsHtml5StreamParser::DoDataAvailable [parser/html/nsHtml5StreamParser.cpp:682]
nsHtml5DataAvailable::Run [parser/html/nsHtml5StreamParser.cpp:721]
nsThread::ProcessNextEvent [xpcom/threads/nsThread.cpp:547]
NS_ProcessNextEvent_P [nsThreadUtils.cpp:250]
nsThread::ThreadFunc [xpcom/threads/nsThread.cpp:262]
_pt_root [nsprpub/pr/src/pthreads/ptthread.c:231]
libpthread.so.0 + 0x5ab5

Also happens in:
REFTEST TEST-START | file:///home/cltbld/talos-slave/tryserver_fedora-debug_test-crashtest/build/reftest/tests/gfx/tests/crashtests/122875-1.html
###!!! ASSERTION: Cannot be used on a background thread!: 'Error', file /builds/slave/tryserver-linux-debug/build/modules/libpref/src/nsPrefService.cpp, line 871
nsPrefService::CheckAndLogBackgroundThreadUse [modules/libpref/src/nsPrefService.cpp:873]
nsPrefBranch::GetCharPref [modules/libpref/src/nsPrefBranch.cpp:219]
nsPrefService::GetCharPref [modules/libpref/src/nsPrefService.h:60]
nsJapaneseToUnicode::setMapMode [intl/uconv/ucvja/nsJapaneseToUnicode.cpp:64]
nsShiftJISToUnicode::nsShiftJISToUnicode [intl/uconv/ucvja/nsJapaneseToUnicode.h:61]
nsShiftJISToUnicodeConstructor [intl/uconv/src/nsUConvModule.cpp:436]
mozilla::GenericFactory::CreateInstance [GenericFactory.cpp:49]
nsComponentManagerImpl::CreateInstanceByContractID [xpcom/components/nsComponentManager.cpp:1284]
CallCreateInstance [nsComponentManagerUtils.cpp:170]
nsCreateInstanceByContractID::operator() [nsComponentManagerUtils.cpp:210]
nsCOMPtr<nsIUnicodeDecoder>::assign_from_helper [nsCOMPtr.h:1272]
nsCOMPtr<nsIUnicodeDecoder>::operator= [nsCOMPtr.h:731]
nsCharsetConverterManager::GetUnicodeDecoderRaw [intl/uconv/src/nsCharsetConverterManager.cpp:233]
nsHtml5MetaScanner::tryCharset [parser/html/nsHtml5MetaScannerCppSupplement.h:125]
nsHtml5MetaScanner::tryCharset [parser/html/nsHtml5MetaScanner.cpp:699]
nsHtml5MetaScanner::stateLoop [parser/html/nsHtml5MetaScanner.cpp:327]
nsHtml5MetaScanner::sniff [parser/html/nsHtml5MetaScannerCppSupplement.h:69]
nsHtml5StreamParser::SniffStreamBytes [parser/html/nsHtml5StreamParser.cpp:451]
nsHtml5StreamParser::DoDataAvailable [parser/html/nsHtml5StreamParser.cpp:682]
nsHtml5DataAvailable::Run [parser/html/nsHtml5StreamParser.cpp:721]
nsThread::ProcessNextEvent [xpcom/threads/nsThread.cpp:547]
NS_ProcessNextEvent_P [nsThreadUtils.cpp:250]
nsThread::ThreadFunc [xpcom/threads/nsThread.cpp:262]
_pt_root [nsprpub/pr/src/pthreads/ptthread.c:231]
libpthread.so.0 + 0x5ab5

Comment 1

[Shawn Wilsher :sdwilsh]

Given the stacks, I suspect this is a regression from the HTML5 parser doing things off of the main thread.

Comment 2

[Shawn Wilsher :sdwilsh]

Also happens in:
REFTEST TEST-START | file:///Users/cltbld/talos-slave/tryserver_snowleopard-debug_test-reftest/build/reftest/tests/layout/reftests/bugs/109735-1.html

REFTEST TEST-START | file:///Users/cltbld/talos-slave/tryserver_snowleopard-debug_test-reftest/build/reftest/tests/layout/reftests/bugs/109735-1-ref.html

REFTEST TEST-START | file:///Users/cltbld/talos-slave/tryserver_snowleopard-debug_test-reftest/build/reftest/tests/layout/reftests/bugs/116882-1.html

REFTEST TEST-START | file:///Users/cltbld/talos-slave/tryserver_snowleopard-debug_test-reftest/build/reftest/tests/layout/reftests/bugs/116882-1-ref.html

Comment 3

[Benjamin Smedberg]

Can we just remove the pref? From what I can tell, this pref is set to one value on Windows, a different value on OS/2, and not set on other platforms. It seems really strange to me that we need platform-specific decoding which might affect the web, but if we really do, let's just hardcode it an skip having a pref altogether?

Comment 4

[Simon Montagu :smontagu]

(In reply to comment #3)
> Can we just remove the pref? 

See discussion in bug 108136

Comment 5

[Benjamin Smedberg]

I really don't think that's a convincing argument, can we remove the pref?

Comment 6

[Simon Montagu :smontagu]

(In reply to comment #5)
> I really don't think that's a convincing argument, can we remove the pref?

Which part isn't convincing, having the platform difference in the first place or using a pref instead of platform ifdefs?

We need input from Japanese experts here.

Comment 7

[Benjamin Smedberg]

It's 2010. If we still need the platform ifdefs because of bad default fonts, that sucks but whatever. It's the pref that should go.

Comment 8

[Shawn Wilsher :sdwilsh]

(In reply to comment #6)
> Which part isn't convincing, having the platform difference in the first place
> or using a pref instead of platform ifdefs?
IMO, using a pref instead of platform ifdefs.

Comment 9

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(PTO: 5/2-5/10)]

The first reason of bug 108136 comment 33 which is for Unix users makes sense for me. Any fonts are not bad because each font is created for a platform.

However, I'm not sure whether that is still problem for Unix users too. All of Linux distributions are using UTF-8 for their default encoding now. And even if that is still problem, such troubles should happen on other applications too. I don't think that it should be fixed by prefs only on Gecko.

Comment 10

[Henri Sivonen (:hsivonen)]

Making the pref go away is the most important thing, but it's totally bogus for Shift_JIS in Web content to decode differently in a DOM-exposed way depending on the platform the browser runs on.

Comment 11

[Henri Sivonen (:hsivonen)]

Attachment: Untested fix

Comment 12

[Masatoshi Kimura [:emk]]

Comment on attachment 471822 [details] [diff] [review]
Untested fix

> +  // HTML5 says to use Windows-31J instead of the real Shift_JIS for decoding
> +  // XXX do we really want this for ISO-2022-JP and EUC-JP, too?
Absolutely required. CP51932 and CP50220 are going to be registered for this purpose.

Comment 13

[Henri Sivonen (:hsivonen)]

Attachment: Fix using #ifdef instead of a pref

(In reply to comment #12)
> Comment on attachment 471822 [details] [diff] [review]
> Untested fix
> 
> > +  // HTML5 says to use Windows-31J instead of the real Shift_JIS for decoding
> > +  // XXX do we really want this for ISO-2022-JP and EUC-JP, too?
> Absolutely required. CP51932 and CP50220 are going to be registered for this
> purpose.

OK. I removed the XXX comment.

I didn't modify japanese.map since it is generated code. I'm not sure if part of it is now dead code.

Comment 14

[Simon Montagu :smontagu]

Comment on attachment 473481 [details] [diff] [review]
Fix using #ifdef instead of a pref

So this is going to change behaviour on platforms other than OS/2 and Windows, correct? I tested (on Mac) that round-tripping still works, and I guess that is the important thing.

Comment 15

[Henri Sivonen (:hsivonen)]

(In reply to comment #14)
> So this is going to change behaviour on platforms other than OS/2 and Windows,
> correct?

I'm not sure, but having other platforms differ from Windows in how they decode bytes to UTF-16 would be bogus, so if Mac and Linux weren't doing the right thing already, the change should be good.

Landed: http://hg.mozilla.org/mozilla-central/rev/570dc03cbf24