Closed
Bug 563536
Opened 15 years ago
Closed 15 years ago
[HTML5] "ASSERTION: wrong thread" [@ nsIOService::NewURI] with nsHtml5MetaScanner::tryCharset on the stack
Categories
(Core :: DOM: HTML Parser, defect, P1)
Tracking
()
RESOLVED
FIXED
Tracking | Status | |
---|---|---|
blocking2.0 | --- | final+ |
People
(Reporter: jruderman, Assigned: hsivonen)
References
Details
(Keywords: assertion, testcase)
Attachments
(2 files, 2 obsolete files)
(deleted),
text/plain
|
Details | |
(deleted),
patch
|
smontagu
:
review+
|
Details | Diff | Splinter Review |
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
Flags: in-testsuite+
Comment 1•15 years ago
|
||
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...
Blocks: html5-parsing
Comment 2•15 years ago
|
||
Is this why I got this crash?
http://crash-stats.mozilla.com/report/index/bp-ae6368f5-a6f8-4b7c-8f33-ef9102100429
mw22: fwiw, hsivonen looked at your stack and it's just a bug in the converter. he'll file it after lunch.
and he filed bug 563618 for it, i've cc'd you
Assignee | ||
Comment 5•15 years ago
|
||
(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.
Assignee | ||
Comment 6•15 years ago
|
||
(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?
Assignee | ||
Updated•15 years ago
|
Priority: -- → P1
Summary: "ASSERTION: wrong thread" [@ nsIOService::NewURI] with nsHtml5MetaScanner::tryCharset on the stack → [HTML5] "ASSERTION: wrong thread" [@ nsIOService::NewURI] with nsHtml5MetaScanner::tryCharset on the stack
Comment 7•15 years ago
|
||
> 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?
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•15 years ago
|
||
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).
Assignee | ||
Comment 10•15 years ago
|
||
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?
Assignee | ||
Comment 11•15 years ago
|
||
(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.
Assignee: nobody → hsivonen
Status: NEW → ASSIGNED
Assignee | ||
Comment 12•15 years ago
|
||
s/This fully fix/This *doesn't* fully fix/
Assignee | ||
Comment 13•15 years ago
|
||
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.
Assignee | ||
Comment 14•15 years ago
|
||
Attachment #443829 -
Attachment is obsolete: true
Assignee | ||
Comment 15•15 years ago
|
||
Attachment #444387 -
Attachment is obsolete: true
Assignee | ||
Comment 16•15 years ago
|
||
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.
Attachment #444397 -
Flags: review?(smontagu)
Assignee | ||
Comment 17•15 years ago
|
||
s/utf-c/utf-8/
Assignee | ||
Comment 18•15 years ago
|
||
smontagu, ping? I think it would be good to land this before the alpha.
Updated•15 years ago
|
Attachment #444397 -
Flags: review?(smontagu) → review+
Assignee | ||
Comment 19•15 years ago
|
||
Thanks. Pushed:
http://hg.mozilla.org/mozilla-central/rev/8d6db7f8fe09
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Updated•14 years ago
|
blocking2.0: ? → final+
Comment 20•14 years ago
|
||
(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.
Assignee | ||
Comment 21•14 years ago
|
||
(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?
You need to log in
before you can comment on or make changes to this bug.
Description
•