241128 - Thunderbird and Seamonkey Mail crash on some messages with JS enabled

Status: RESOLVED FIXED

(MailNews Core :: Backend, defect, P1)

Description

[Scott MacGregor]

Talkback ID: tb26235H

 CNavDTD::HandleToken
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/htmlparser/src/CNavDTD.cpp,
line 853]
CNavDTD::BuildModel
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/htmlparser/src/CNavDTD.cpp,
line 520]
nsParser::BuildModel
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/htmlparser/src/nsParser.cpp,
line 1899]
nsParser::ResumeParse
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/htmlparser/src/nsParser.cpp,
line 1763]
nsParser::Parse
[c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/htmlparser/src/nsParser.cpp,
line 1659]
gkparser.dll + 0x29374 (0x604d9374)

The trick is to have JS enabled in the message pane. The messages that cause
this crash have JS in them that seem to be trying to do some interesting things.

I can't post some of the example messages that cause this but David and I can
give dbaron one of these examples.

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Any chance you could send one of the examples?

Comment 2

[David :Bienvenu]

the tokenizer has been deleted out from under us, it seems. I'll send you one of
the messages...

    [E] FMR: Free memory read in CNavDTD::BuildModel(nsIParser *,nsITokenizer
*,nsITokenObserver *,nsIContentSink *) {1 occurrence}
        Reading 4 bytes from 0x0c79f2f0 (4 bytes at 0x0c79f2f0 illegal)
        Address 0x0c79f2f0 is at the beginning of a 84 byte block
        Address 0x0c79f2f0 points to a C++ new block in heap 0x02420000
        Thread ID: 0x8ac
        Error location
        CNavDTD::BuildModel(nsIParser *,nsITokenizer *,nsITokenObserver
*,nsIContentSink *) [CNavDTD.cpp:509]
        nsParser::BuildModel(void) [nsParser.cpp:1894]
        nsParser::ResumeParse(int,int,int) [nsParser.cpp:1761]
        nsParser::Parse(nsAString const&,void *,nsACString
const&,int,int,nsDTDMode) [nsParser.cpp:1644]
        nsHTMLDocument::WriteCommon(nsAString const&,int) [nsHTMLDocument.cpp:2268]
        nsHTMLDocument::ScriptWriteCommon(int) [nsHTMLDocument.cpp:2354]
        nsHTMLDocument::Write(void) [nsHTMLDocument.cpp:2380]
        XPTC_InvokeByIndex [xptcinvoke.cpp:101]
        XPCWrappedNative::CallMethod(XPCCallContext&,CallMode::XPCWrappedNative)
[xpcwrappednative.cpp:2027]
        XPC_WN_CallMethod(JSContext *,JSObject *,UINT,long *,long *)
[xpcwrappednativejsops.cpp:1287]
        Allocation location
        new(UINT)      [new.cpp:23]
        NS_NewHTMLTokenizer(nsITokenizer * *,int,eParserDocType,eParserCommands)
[nsHTMLTokenizer.cpp:113]
        CParserContext::GetTokenizer(int,nsITokenizer *&) [CParserContext.cpp:177]
        nsParser::Tokenize(int) [nsParser.cpp:2541]
        nsParser::ResumeParse(int,int,int) [nsParser.cpp:1760]
        nsParser::Parse(nsAString const&,void *,nsACString
const&,int,int,nsDTDMode) [nsParser.cpp:1644]
        nsHTMLDocument::WriteCommon(nsAString const&,int) [nsHTMLDocument.cpp:2268]
        nsHTMLDocument::ScriptWriteCommon(int) [nsHTMLDocument.cpp:2354]
        nsHTMLDocument::Write(void) [nsHTMLDocument.cpp:2380]
        XPTC_InvokeByIndex [xptcinvoke.cpp:101]
        XPCWrappedNative::CallMethod(XPCCallContext&,CallMode::XPCWrappedNative)
[xpcwrappednative.cpp:2027]
        XPC_WN_CallMethod(JSContext *,JSObject *,UINT,long *,long *)
[xpcwrappednativejsops.cpp:1287]
        js_Invoke      [jsinterp.c:1281]
        js_Interpret   [jsinterp.c:3366]
        js_Execute     [jsinterp.c:1507]
        JS_EvaluateUCScriptForPrincipals [jsapi.c:3569]
        nsJSContext::EvaluateString(nsAString const&,void *,nsIPrincipal *,char
const*,UINT,char const*,nsAString&,int *) [nsJSEnvironment.cpp:942]
        nsScriptLoader::EvaluateScript(nsScriptLoadRequest *,nsString const&)
[nsScriptLoader.cpp:658]
    Free location
        delete(void *) [delop.cpp:6]
        nsHTMLTokenizer::`scalar deleting destructor'(UINT) [gkparser.dll]
        nsHTMLTokenizer::Release(void) [nsHTMLTokenizer.cpp:122]
        CParserContext::~CParserContext(void) [CParserContext.cpp:142]
        CParserContext::`scalar deleting destructor'(UINT) [gkparser.dll]
        nsParser::ResumeParse(int,int,int) [nsParser.cpp:1832]
        nsParser::ContinueParsing(void) [nsParser.cpp:1359]
        CSSLoaderImpl::SheetComplete(SheetLoadData *,int) [nsCSSLoader.cpp:1530]
        CSSLoaderImpl::LoadSheet(SheetLoadData *,StyleSheetState)
[nsCSSLoader.cpp:1367]
        CSSLoaderImpl::LoadStyleLink(nsIContent *,nsIURI *,nsAString
const&,nsAString const&,nsIParser *,int&,nsICSSLoaderObserver *)
[nsCSSLoader.cpp:1719]
        nsStyleLinkElement::UpdateStyleSheet(nsIDocument *,nsICSSLoaderObserver
*) [nsStyleLinkElement.cpp:311]
        HTMLContentSink::ProcessLINKTag(nsIParserNode const&)
[nsHTMLContentSink.cpp:4016]
        HTMLContentSink::AddLeaf(nsIParserNode const&) [nsHTMLContentSink.cpp:3194]
        HTMLContentSink::AddHeadContent(nsIParserNode const&)
[nsHTMLContentSink.cpp:3162]
        CNavDTD::AddHeadLeaf(nsIParserNode *) [CNavDTD.cpp:3839]
        CNavDTD::HandleStartToken(CToken *) [CNavDTD.cpp:1832]
        CNavDTD::HandleToken(CToken *,nsIParser *) [CNavDTD.cpp:1019]
        CNavDTD::BuildModel(nsIParser *,nsITokenizer *,nsITokenObserver
*,nsIContentSink *) [CNavDTD.cpp:511]

Comment 3

[David :Bienvenu]

looks like the js is doing a document.write, and we've already deleting the
tokenizer...

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Any chance you could attach longer versions of the stacks in comment 2? 
(Preferably the whole stack, if purify has it.  And probably in an attachment
rather than pasted in the bug...)

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to comment #2)
>         CSSLoaderImpl::SheetComplete(SheetLoadData *,int) [nsCSSLoader.cpp:1530]
>         CSSLoaderImpl::LoadSheet(SheetLoadData *,StyleSheetState)
> [nsCSSLoader.cpp:1367]

I'm guessing you have revision 3.184.  Why did NS_NewChannel fail?

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

It could have something to do with

WARNING: unknown protocol in nsScriptSecurityManager::CheckLoadURI, file
/builds/trunk/mozilla/caps/src/nsScriptSecurityManager.cpp, line 1286

as well.  I should also be able to figure out why NS_NewChannel failed myself...

Comment 7

[Boris Zbarsky [:bzbarsky]]

> WARNING: unknown protocol in nsScriptSecurityManager::CheckLoadURI, file
> /builds/trunk/mozilla/caps/src/nsScriptSecurityManager.cpp, line 1286

If it's some random bogus protocol (eg <link rel="stylesheet" href="foo:bar">)
that could give this warning and cause the channel creation to fail.

Marking dependent on bug 84582, but we may need to put in an interim hack like
we did for bug 197015....

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: end of NSPR log (nsCSSLoader:5)

Comment 9

[David :Bienvenu]

Attachment: complete stack of FMR

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

So, actually, I suspect that what's going wrong here is that a relative URL in
an HTML file forwarded in an email isn't working correctly, despite the presence
of a <BASE HREF="...">, and if we fix that, we might avoid the crash.  Or
something like that...

Comment 11

[Boris Zbarsky [:bzbarsky]]

Are those URIs with a spec of "" I'm seeing in the log?

I assume there's some HTML to that effect or that some protocol handler impl is
royally screwing up?

Comment 12

[Boris Zbarsky [:bzbarsky]]

Allow me to correct that to "some protocol handler impl IS royally screwing up"
(quite apart from whether the HTML has "" as URIs).

Comment 13

[David :Bienvenu]

note that this happens in both imap and local mailboxes - is it mime that's
messing up, generating incorrect urls?

Comment 14

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I think part of the problem is that in HTMLContentSink::ProcessBaseHref , mBody
is non-null, even though the document begins <BASE ...><html><head>....

Comment 15

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch to set _baseHref on LINK, META, STYLE, and SCRIPT

This fixes the crash, but there are at least two other underlying problems here
(see comment 7 and comment 14).

Comment 16

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch to set _baseHref on LINK, META, STYLE, and SCRIPT

This one is more consistent about where to add the new function calls.

Comment 17

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(Actually, comment 14 isn't a bug; it's the result of the way mail displays HTML
inline.)

Comment 18

[Johnny Stenback (:jst)]

Comment on attachment 146638 [details] [diff] [review]
patch to set _baseHref on LINK, META, STYLE, and SCRIPT

r+sr=jst

Comment 19

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 146638 [details] [diff] [review]
patch to set _baseHref on LINK, META, STYLE, and SCRIPT

This is a long-standing bug that can cause some rather serious problems with
pages with multiple BASE elements or with HTML attached to email.

Comment 20

[David :Bienvenu]

fix checked into m4 branch - this fixes the problem on that branch.

Comment 21

[Boris Zbarsky [:bzbarsky]]

Fun stuff.  So remaining issues are, as I see them:

1)  What produced bogus URIs instead of throwing an exception on newURI()?  This
    should be fixed.
2)  Why are we actually crashing?  That is, could we reproduce this outside
    mailnews by using URIs that are valid uris with unknown schemes but fail to
    open the channel?

We can probably spin off separate bugs for those if people want....

Comment 22

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Fix checked in to trunk, 2004-04-20 16:41:56 -0700.
Fix checked in to MOZILLA_1_7_BRANCH, 2004-04-20 19:40:15 -0700.

Comment 23

[David :Bienvenu]

we have a similar e-mail that still crashes, so we'll either need to spin off a
new bug, or re-open this one...I've sent it to dbaron. In that message, the
baseHREF is empty, which might explain why the attached fix didn't fix this
problem...

Comment 24

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Separate bug.  I filed bug 241254 (and attached a simple testcase).