182021 - Regression - Some HTML messages display blank in Simple HTML mode

Status: RESOLVED FIXED

(Core :: DOM: HTML Parser, defect)

Description

[Ben Bucksch (:BenB)]

See bug 138125 comment 27.
Probably caused by bug 177994.

Comment 1

[harishd]

I'm not surprised :-). May be Daniel's patch, in bug 180996, might fix the
problem :-/. 

Comment 2

[Ben Bucksch (:BenB)]

> I'm not surprised :-)

em...

> bug 180996

No, that bug is about the plaintext serializer, while this one is about the
sanitizer.

Comment 3

[harishd]

> while this one is about the sanitizer

oh. But, may be this is a similar issue ( mParserNode related ).

Comment 4

[esther]

Note to test the html messages attached to bug 182021 when fixed since these
messages regressed from displaying in simple html with 11-18 and 11-19 trunk
builds to not displaying in simple html in 11-20 trunk builds. All platforms. 
cc'in esther 

Comment 5

[harishd]

Could some one tell me the steps to reproduce the problem?

Comment 6

[Ere Maijala (slow)]

Actually, it's bug 138125 where the messages are attached. To display such a
message, you'll need to manually add it to the folder file (as we still suffer
from bug 11076). Easy way is to create a local folder in MailNews, shut it down,
find the corresponding file under profile directory and replace it with the
message(s) file. Start Mozilla, view the message in different modes (View ->
Message Body As -> *). 

Comment 7

[harishd]

Looks like CNavDTD::AddHeadLeaf() calls OpenHead() with an incorrect node!. That
is, OpenHead() is called ( done so to switch to head context in the sink -
HTMLContentSink ) on the sink for every child of head. This was not a problem,
before my checkin, because we used to call CloseHead() with the same child node
and therefore was able to match the open/close nodes in the sink rather
mozSanitizingSerializer. However, after my checkin a call to sink's CloseHead
always resulted in closing a matching start head tag, if there was one, and
therefore resulted in not closing the head children that got opened up by
OpenHead() calls. This is what, I believe, is the root cause of the regression.
Basically, CNavDTD needs spanking :->

Comment 8

[Michiel van Leeuwen (email: mvl+moz@)]

*** Bug 181733 has been marked as a duplicate of this bug. ***

Comment 9

[harishd]

Attachment: patch v1.0

With this patch CNavDTD ( and potentially other DTDs ) can add head content
without worrying about manually switching contexts ( body context -> head
context ) in the content sink. That is, CNavDTD used to call OpenHead and
CloseHead methods, with incorrect node info!, just to make the context switch
but with the patch the context switch happens automatically. This clean up
fixes the issue raised in this bug.

Comment 10

[Johnny Stenback (:jst)]

Comment on attachment 107921 [details] [diff] [review]
patch v1.0

sr=jst if you rename AddHeadLeaf() to AddHeadContent() (as we discussed in
person).

Comment 11

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Comment on attachment 107921 [details] [diff] [review]
patch v1.0

>Index: content/base/src/mozSanitizingSerializer.cpp
>===================================================================
>+mozSanitizingHTMLSerializer::AddHeadLeaf(const nsIParserNode& aNode)
>+{
>+  nsresult rv = OpenContainer(aNode);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+  return CloseContainer(nsHTMLTag(aNode.GetNodeType()));

Why isn't there a call to AddLeaf in between those calls?

Otherwise the patch looks good.

Comment 12

[harishd]

>Why isn't there a call to AddLeaf in between those calls?

Good question. The code in mozSanitizingSerializer::AddLeaf calls
DoOpenContainer ( Ben can explain why :-) ) if the node type is not "text", or
"entity", or "whitespace" and therefore could potentially leave a node unclosed.
Anyway, I should probably add the call to AddLeaf if the node type was "text",
or "entity", or "whitespace" and should call OpenContainer and CloseContainer
otherwise.

Comment 13

[Ben Bucksch (:BenB)]

> The code in mozSanitizingSerializer::AddLeaf calls
> DoOpenContainer ( Ben can explain why :-) )

Actually, I can't :-(. I don't know why the call is there. I didn't know the
APIs (mostly looked at nsPlaintextSerializer), nor anyone who does.

BTW: If you change something structurally, make sure that you don't regress the
code, it's security-relevant. Please test Mailnews' View|Message Body As|Simple
HTML feature thoroughly.

Comment 14

[harishd]

Attachment: patch v1.1

Addresses jst's and heikki's concerns. Ben, all the modes ( original | simple
html | plain text ) worked fine with the patch.

Comment 15

[Christian Franke]

[Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.3a) Gecko/20021212]

comment 5: After upgrading from 1.2 to 1.3a, the this bug was very easy to
reproduce:

1. Open Mozilla Mail
2. Compose
3. Type some simple message text like "Invisible in Simple HTML"
4. Ctrl+S and close the Edit Window
5. View|Message Body as|Simple Html
6. Select the new message in the "Drafts" folder

Actual Results: Message invisible in the Message Pane and Message Window
Expected Result: Message visible
Reproducible: always
Workaround: Select Original HTML or Plain Text

Sending the message as HTML or Text/HTML: the received message is still invisible.

Newer got this bug in 1.2b and 1.2.

Comment 16

[Daniel Bratell]

Comment on attachment 109158 [details] [diff] [review]
patch v1.1

Can the patch be checked in now?

Comment 17

[harishd]

Comment on attachment 109158 [details] [diff] [review]
patch v1.1

Carrying forward r/sr=.

Comment 18

[harishd]

Fixed.

Comment 19

[Ere Maijala (slow)]

Great, seems to work again :)

Comment 20

[[e-mail] (gone - use [e-mail] instead)]

*** Bug 178050 has been marked as a duplicate of this bug. ***