Closed Bug 593046 Opened 14 years ago Closed 14 years ago

nsJapaneseToUnicode uses the pref service off of the main thread

Categories

(Core :: Internationalization, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- final+

People

(Reporter: sdwilsh, Assigned: hsivonen)

References

Details

Attachments

(1 file, 1 obsolete file)

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
Given the stacks, I suspect this is a regression from the HTML5 parser doing things off of the main thread.
Blocks: 482918
blocking2.0: --- → final+
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
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?
(In reply to comment #3)
> Can we just remove the pref? 

See discussion in bug 108136
I really don't think that's a convincing argument, can we remove the pref?
(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.
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.
(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.
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.
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.
Attached patch Untested fix (obsolete) — Splinter Review
Attachment #471822 - Attachment is patch: true
Attachment #471822 - Attachment mime type: application/octet-stream → text/plain
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.
(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.
Assignee: smontagu → hsivonen
Attachment #471822 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #473481 - Flags: review?
Attachment #473481 - Flags: review? → review?(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.
Attachment #473481 - Flags: review?(smontagu) → review+
(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
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: