Closed Bug 16550 Opened 25 years ago Closed 25 years ago

[DOGFOOD][BLOCKER] [Regression] Can't reply to messages using html

Categories

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

Product:
Core
Component:
DOM: HTML Parser
Platform:
All
Windows NT
Type:
defect

Tracking

()

Status:
VERIFIED FIXED

People

(Reporter: mscott, Assigned: mscott)

References

Details

Attachments

(3 files)

file generated during a reply
571 bytes, text/html
Details
Patch for webshell change to fix the problem
2.15 KB, patch
Details | Diff | Splinter Review
Patch for MN regression
4.10 KB, patch
Details | Diff | Splinter Review 
In todays build (10/15), replying to a message using html always fails. The
compose window comes up for me but the editor window is always blank. Actually
it's worse than blank. It actually captures whatever content was underneath the
compose window.

Lisa, can we look into adding a reply to test to our mailnews smoketest stuff? I
think we missed this today and the tree opened. I'm trying to email yesterday's
hook now....
Interesting.  Just checked with Laurel - she can reply in html using today's
release (2nd respin) builds.
Rich, if I back out your changes to nsMimeXULEmitter.cpp, I at least get a blank
editor window instead of one that shows garbage (grabbing content from
underneath the window).

So this is a starting point.

Seth sees the same problem on Linux.
My XUL emitter change it not the problem here. In either case, you are not
getting an Editor instantiated, the "garbage pixels" is just a random behavior.

- rhp
Are you sure Rich? Why is it with your change, I get garbage pixels...without
your change, I get a blank compose window. Something is fishy somewhere that
backing that out made a difference. Maybe it's influencing a race condition
somewhere. Obviously backing out your change doesn't fix the problem as I still
get a blank compose window, it just isn't garbage bits.
The XUL emitter change is the one where I'm doing the nsEscapeHTML() call now,
right?

- rhp
Hey Rich, I was just stepping through your change to nsMimeXULEmitter and I saw
a small optimization that would cut down on the number of strings you allocate.

You had:
    nsString  workString(curName);
    char      *tName;

    workString.Trim("\"");
    tName = workString.ToNewCString();
    workName = nsEscapeHTML(tName);
    if (workName)
    {
      PR_FREEIF(tName);
    }
    else
    {
      workName = tName;
    }
  }

I think you'll reduce at least one copy of the string if you use a nsAutoCString
here instead and pass that directly to nsEscapeHTML instead of converting to a
nsString then having it convert back to a char * just to pass into nsEscapeHTML.

So the code snippet could look like:
    nsCAutoString  workString(curName);
    char      *tName;

    workString.Trim("\"");
    workName = nsEscapeHTML(workString);
    if (!workName)
    {
      workName = workString;
    }
This saves an extra allocation/free and it shortens the code too. Just my little
post code reviewer tip...

Anyway, I'm stepping through the debugger now...
IF you start typing into the body, you can see xxx wrote: but everything is
displayed in double, event what your are typing (this part should be an Editor
problem). When I output the content as HTML or XIF, the original body is
missing!
Rich, I don't know why backing out that file made me get a legit blank screen. I
set a breakpoint and that code doesn't even get called, when replying to a
message.

So it's definetly something else.
Well looks like nothing changed in compose yesterday and hardly anything
changed in mime. So it looks like it something outside of mailnews that caused
the regression. I'm scanning editor changes and webshell changes now.
the generated file (nscomp.html) that we load into ender seems ok for me. By ok
I mean is like it always been. But when I load it into the browser, nothing is
displayed. same for ender. I will attach the file...
The attached file load correctly in 4.x. Seems to be a layout problem!
Assignee: ducarroz → rickg
Component: Composition → Parser
Product: MailNews → Browser
Summary: [BLOCKER] Can't reply to messages using html → [BLOCKER] [Regression] Can't reply to messages using html
Severity: normal → blocker
Priority: P3 → P1
this is a layout bug, in that the browser cannot load this document either.  the
HTML in the test case is aweful, but Nav can lay it out so we should too in
quirks mode.
changing product to parser.  Marking P1, blocker, regression.
I'd like to help narrow the problem down for the layout guys a bit more. We know
that sometime yesterday layout lost the ability to load our reply-to message
files.

But we do know that we can view messages in the message pane.

Rich/Jean-Francois, can you list here for rickg's benefit how the html content
differs between displaying a message and replying to a message. i.e. Doesn't
compose insert some of its own information into the file on top of the html
generated by the mime parser?
We just take the whole original body (HTML or plain text) and we add arround the
following.


<br><br>chris kritzer test3 wrote:<br><BLOCKQUOTE TYPE=CITE><html>

...then we put the original body here...


</html></BLOCKQUOTE>

The result is that we can have several HTML tag! When we quote an plain text
body, we also add a <PRE> tag around the body.
Blocks: 16563
No longer blocks: 16563
Rick, just a little extra info to help narrow this down. Both viewer and
apprunner fail to load this page for me. On a build from 10/14 (morning),
both apprunner and viewer can load Jean-Francois's attachement.
Got it!! On a hunch, I backed on Radha's webshell changes because they involved
altering document load notifications. Sure enough that did it. replying to
messages now works again.

I'm going to re-assign this bug back to me and take rickg off of this since it
really doesn't involve him. I'll work with radha to figure out how her stuff
broke us.
Assignee: rickg → mscott
Re-assigning to me, cc'ing radha since her changes in the webshell broke this.
I'll try to work with her to figure out where the breakage is coming from.
Radha, in your changes to nsWebShell::DoLoadURL yesterday, you introduced a new
code path that broke things I think.

The code used to do the following:
1) If the url has a dest anchor and a pres context then do some stuff andcall
   OnEndDocumentLoad and return.
2) Or, if aType == nsISessionHistory::LOAD_HISTORY then do some stuff and call
   OnEndDocumentLoad
3) If neither of these cases is true, then just fall through and load the url.

After your change the code flow looks like:
1) If url has a destination anchor do some stuff
2) if aType == nsISessionHistory::LOAD_HISTORY do some stuff.
3) call OnEndDocumentLoad and return

Notice that now, even if (1) or (2) isn't true, we still call
OnEndDocumentLoad and return.

This is what broke mailnews today. I'm surprised we didn't get caught elsewhere.

Anway, there are two options as I see them
1) We can back out the change to nsWebShell involving ::DoLoadURL. It looked
to me that the change was having OnendDocumentLoad and the return call after
these two if clauses like I illustrated above.
2) Or if you want, I've modified the methods such that it looks more like it
used to (same effect as backing it out) where we call OnEndDocumentLoad and
return inside each of the if clauses so we don't do that for all urls
we try to load.

This is a P1 stopper and we really need to get this fixed before Monday morning
when the QA builds go off. So if you can look at this and let me know how you'd
like me to proceed before then, I'd appreciate it.
Status: NEW → ASSIGNED
Adding Leaf to the cc list. Leaf, I haven't been ble to get a hold of radha yet
to figure out how she wants to handle this bug. I have a fix in my tree but
would need her to review it.

We certainly want to keep the tree closed tomorrow mornig if she doesn't get a
chance to look at it before then before we open the tree up. Thanks.
I'm looking at my changes. I'll reply soon. Can you send me your patches?
Summary: [BLOCKER] [Regression] Can't reply to messages using html → [DOGFOOD][BLOCKER] [Regression] Can't reply to messages using html
Radha, I'm attaching my patch to this bug report now. Although to my eye, my
changes make the function look pretty much like it did before you changed it. So
it could be just as safe to back out the changes to nsWebShell::DoLoadURL.
scott,
I have attached patches which  moves the OnStartDocumentLoad() also inside the
if so that it affects only the browser. Please try my changes and let me know. I
'll check it in.

Thanks,
Radha
please don't checkin until the tree opens, or until we decide to respin for this
fix.
Thanks Radha, I've actually been trying to save and apply this patch on my
windows box but am having line feed termination problems. It's really strange.
I'm playing with it now.
Radha, did you attach the patch as a text file? I'm having difficulaties
applying the patch on both my linux and windows machines. Running the actual
patch routine is failing.
Can you email me the patch instead? I just can't seem to get this patch to work
on either machine for me. Thanks.
Radha, can you diff the file again but this time pipe it directly into a file?
i.e. do cvs diff -c > myFilePatch.txt

I'm guessing that you selected the diff code in the console window and tried to
paste that into a file which doesn't work. If you try to select the patch text
and copy/paste it, the patch comes out bogus because extra line returns are
entered in the pasted text based on how wide your terminal window was when you
pasted the text. Thanks.
I have checked in the patches
Blocks: 16531
Status: ASSIGNED → RESOLVED
Closed: 25 years ago
Resolution: --- → FIXED
Radha checked in the fix this afternoon. Marking as fixed.
This is working for me using 1999-10-19-08m11 commercial builds.  However, since
I didn't have this extreme problem originally, I'm going to ask someone else to
confirm it...mscott?
Status: RESOLVED → VERIFIED
Sure thing Laural. Marking fixed on 10/19 build...
Blocks: 16950
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: