563536 - [HTML5] "ASSERTION: wrong thread" [@ nsIOService::NewURI] with nsHtml5MetaScanner::tryCharset on the stack

Status: RESOLVED FIXED

(Core :: DOM: HTML Parser, defect, P1)

Description

[Jesse Ruderman]

Attachment: three stack traces

layout/reftests/bugs/142233-1-ref.html triggers 3x:

###!!! ASSERTION: wrong thread: 'NS_IsMainThread()', file /Users/jruderman/mozilla-central/netwerk/base/src/nsIOService.cpp, line 492

Comment 1

[Boris Zbarsky [:bzbarsky]]

nsGREResProperties, sigh...

Can we make that sync-proxy to the main thread as needed?

Of course there's the little matter of bug 204111 in general...

Comment 2

[Martijn Wargers (dead)]

Is this why I got this crash?
http://crash-stats.mozilla.com/report/index/bp-ae6368f5-a6f8-4b7c-8f33-ef9102100429

Comment 3

[timeless]

mw22: fwiw, hsivonen looked at your stack and it's just a bug in the converter. he'll file it after lunch.

Comment 4

[timeless]

and he filed bug 563618 for it, i've cc'd you

Comment 5

[Henri Sivonen (:hsivonen)]

(In reply to comment #1)
> Can we make that sync-proxy to the main thread as needed?

Any chance of populating nsGREResProperties on the main thread but letting the object be accessed directly on the parser thread subsequently?

> Of course there's the little matter of bug 204111 in general...

I already added a mutex to nsCharsetAlias2::GetPreferred when I put the HTML5 parser off the main thread. The mutex is similar to what was proposed over in bug 204111.

Comment 6

[Henri Sivonen (:hsivonen)]

(In reply to comment #1)
> nsGREResProperties, sigh...
> 
> Can we make that sync-proxy to the main thread as needed?

nsGREResProperties doesn't have an XPCOM interface, so XPCOM proxies don't work.

biesi, do you have a suggestion?

Comment 7

[Boris Zbarsky [:bzbarsky]]

> Any chance of populating nsGREResProperties on the main thread but letting the
> object be accessed directly on the parser thread subsequently?

That's what I was proposing, yes.  Specifically, post an event to the main thread to create the nsGREResProperties, and then wait for it to have fired (e.g. we could wait on a condvar here and have that event notify the condvar, or we could busy-wait for the main-thread event loop to get to the event...  The former is likely better, right?

Comment 8

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

The former sounds better, but if the main thread can synchronously proxy to the parser thread, deadlock is possible.

But, it appears to me that this class is doing blocking file IO on the main thread, so sdwilsh might have plans for it.

Comment 9

[Shawn Wilsher :sdwilsh]

Don't have any currently plans.  Sadly, we have a lot of main thread I/O still (but a lot less than we used to).

Comment 10

[Henri Sivonen (:hsivonen)]

Do the charset aliases need to be in a property file in the first place? It's not something that users or redistributors should customize lightly. Wouldn't it be cheaper to read the aliases in as part of libxul by putting the alias data into the data segment of the shared library?

Comment 11

[Henri Sivonen (:hsivonen)]

Attachment: Load the property file on the main thread

(In reply to comment #7)
> > Any chance of populating nsGREResProperties on the main thread but letting the
> > object be accessed directly on the parser thread subsequently?
> 
> That's what I was proposing, yes.  Specifically, post an event to the main
> thread to create the nsGREResProperties, and then wait for it to have fired
> (e.g. we could wait on a condvar here and have that event notify the condvar,

I did this (except with a Monitor instead of CondVar). This fully fix bug 204111, because the solution may deadlock depending on what the calling thread does relative to the main thread. I claim it's not going to deadlock in the HTML5 parser case, because nsHtml5StreamParser calls into charset alias code so early in the parse that the main thread doesn't want to acquire mTokenizerMutex on the same nsHtml5StreamParser at that point yet.

Comment 12

[Henri Sivonen (:hsivonen)]

s/This fully fix/This *doesn't* fully fix/

Comment 13

[Henri Sivonen (:hsivonen)]

Either there was unrelated failure or the patch caused a deadlock on Mac tryserver.

I'm trying on tryserver again after rebasing. However, if it turns out that the patch deadlocks on the Mac tinderbox, would it be OK to put the alias data in the shared library itself?

I'm thinking of having an array of string pairs where each pair has a lowercased alias and the preferred name. The array would be lexically sorted by alias and searched with binary search.

Comment 14

[Henri Sivonen (:hsivonen)]

Attachment: Replace nsGREResProperties with arrays baked into the shared library

Comment 15

[Henri Sivonen (:hsivonen)]

Attachment: Replace nsGREResProperties with arrays baked into the shared library, take 2

Comment 16

[Henri Sivonen (:hsivonen)]

Comment on attachment 444397 [details] [diff] [review]
Replace nsGREResProperties with arrays baked into the shared library, take 2

Notes:
 * It seems that try talos is useless for measuring the effect of this patch on startup time. I tried a couple of variants: omitting the value length baked into the third array item in the property tables and shortcutting "utf-c" and "iso-8859-1" in the alias lookup. It is possible that those variants might have been better on some measure but this variant looks the most promising if one pretends to trust the semi-random talos results.
 * The patch goes ahead and removes all uses of nsGREResProperties in addition to the charset alias use. At least the Unix platform charset code tried to be thread-safe but would have suffered from the same problem as the charset alias code if actually called off-the-main thread.
 * As far as I can tell, the gNLInfo stuff wasn't used anymore and all Unix variants are using unixcharset.properties instead of platform-specific files, so I removed the gNLInfo code path.

Filed separate bugs about suspicions that some of the platform charset code might be obsolete anyway.

Comment 17

[Henri Sivonen (:hsivonen)]

s/utf-c/utf-8/

Comment 18

[Henri Sivonen (:hsivonen)]

smontagu, ping? I think it would be good to land this before the alpha.

Comment 19

[Henri Sivonen (:hsivonen)]

Thanks. Pushed:
http://hg.mozilla.org/mozilla-central/rev/8d6db7f8fe09

Comment 20

[Jean-Marc Desperrier]

(In reply to comment #10)
> Do the charset aliases need to be in a property file in the first place? It's
> not something that users or redistributors should customize lightly.

It's a poor assumption Henri. The situation for Big5 shows it (many Big5 labeled pages are truly Big5-HKSCS, this could be handled in a windows-1252 / ISO-8859-1 way but, although infrequent, other non HKSCS compatible extension of Big5 also exist). This demonstrate hard-coding the identifiers is not flexible enough to handle the various, random things that can be met in some corners of the Internet.

I'd like to open a new bug to try to find a more flexible solution that does not have the problems of nsGREResProperties.

Comment 21

[Henri Sivonen (:hsivonen)]

(In reply to comment #20)
> It's a poor assumption Henri. The situation for Big5 shows it (many Big5
> labeled pages are truly Big5-HKSCS, this could be handled in a windows-1252 /
> ISO-8859-1 way but, although infrequent, other non HKSCS compatible extension
> of Big5 also exist). This demonstrate hard-coding the identifiers is not
> flexible enough to handle the various, random things that can be met in some
> corners of the Internet.

How would a would-be customizer decide which extended Big5 flavor to map "Big5" to? If Big5 means different things to different people with differently configured browsers, wouldn't that mean siloing some corners of the Internet depending on browser customization? Are the HKSCS-incompatible flavors of Big5 used on the Web? How are they labeled? Does Gecko's "Big5" decoder support HKSCS-incompatible extensions now?

From the data you've provided so far, it's not at all clear that customizing Firefox is the right solution to the problem. How do other browsers solve the problem?