I don't know when this bug started, but it's in yesterday's release build, and on the tip today - if you read the msg Chris Hoffman sent out with the ZDNet article, then you can't read any other messages, imap or local. If you read a local msg, you crash in nsMailboxProtocol::ReadMessageResponse because m_channelListener is null. If you try to read an imap message, it loads but does not display, again because in nsImapProtocol::BeginMessageDownLoad, m_channelListener is null. I don't know what's causing this but it's easily reproducible.
I'll look into this a bit and see if I can figure out why the channel listener is not set. I *don't* think it was your fix for the news article forwarding problem, because yesterday's release build had the problem.
OK, the problem is that nsMessenger's mDocShell is null after reading Chris's message, and everything goes downhill after that. Not sure how mDocShell got to be null, unless SetWindow got called. I'll look a little more.
So, SetWindow gets called everytime we load a msg, and after loading the msg in question, we can't get the msg pane from the passed in dom window. I wonder if this could be related to the js changes for clearing out js state on web page loades. I'll see if I can find that change and back it out.
i think this is a dup of bug 38398 which was reopened yesterday
Nope, Brendan's change to dom\src\base\nsGlobalWindow.cpp wasn't it. No more clues.
So this is kind of cool.....here's why this page breaks us.... the body is loaded into an iframe that has a name attribute of "messagepane". before we load a message, we find the right iframe to load it into by searching in the DOM for an element with this name. This page attached to this message contains some JS which does the following: self.name = "parentWin" effectively renaming our iframe from "messagePane" to "parentWin". *doh* So we never find the message pane again in future searches..... clearing regression keyword. This isn't a regression.
+, P1 per mail triage
cc'ing Mitch. Mitch, this is the bug I emailed you about last week. The web page we are laying out in the message pane has JS which says self.name = "parentWin" causing our "messagePane" docshell to get renamed. Supposedly JS is disabled in the message pane. So this shouldn't have had the right capaiblities to execute, right?
JS is not disabled in the message pane...messages can contain scripts, and the scripts will run. I wanted to disable JS in mail by default, and was told I can't do this, but even if it's disabled by default, when turned on it should obviously not be allowed to change the name of the frame it's in. There are probably other similar things which JS in mail should not be allowed to do; the sandbox needs to be tighter or we'll wind up with messages which can mail themselves to people in your addressbook and other fun behaviors. I'm not sure how best to deal with this but I'll give it a look.
cc: Cathy Zhang (QA) since this may have to do with mail security. And js is not disabled by default in the message pane in commercial builds as per another bug report that Mitch is referring to.
to pdt: this is a P1 because someone can affect another user's system and make it so that s/he can't read any other message in that session. Per Marketing and mail team's earlier decision, Netscape builds will have JS turned on by default in the message pane due to other reasons so we can't fix this by turning JS off.
PDT agrees P1
Hey Mitch, it turns out my fix for 51442 fixes this bug as well because now the running JS doesn't have the right capabilities for setting the name of it's containing iframe.
Waiting for the (attachment) webpage that caused this bug!
Re-opening bug. Steps to reproduce problem 1- goto www.zdnet.ch (if you viewsource you will notice a self.name="parentWin"); 2- send the page to yourself. 3- Open the page and then try clicking on any other message 4- since parentWin is the name of the pane now messagepane is lost and you cant view messages. 5- you can still view messages in a separate window by clicking on the thread or pressing Enter.
Does not work on release commercial builds 2000092808/2000092810 on all three platforms (mac,linux,win). Although works on release mozilla build (2000092810) on win, linux and mac.
rtm+, this renders mail unusable. Why is only the mozilla build fixed? Did we forget to check this in on the branch or something?
cleaning up: turning nsbeta3+ to nsbeta-: beta3 has passed.
we don't know why it occurs on netscape but not mozilla builds - a couple possibilities are prefs differences between mozilla and netscape, or diffs between debug and release builds. But the branch is not an issue since we branched long after this bug was marked fixed.
JS in Mailnews is disabled by default in Mozilla, enabled in Netscape 6.
changing to [rtm need info]. We want to rtm+ this but need a patch and code reviews. When those exist, please change back to [rtm+].
If 51442 is fixed, how come that page can still rename its iframe??
What is bug 51442? I don't have permissions to view it :-(((.
*** Bug 55871 has been marked as a duplicate of this bug. ***
Have we lost the battle on this one? Should I mark it rtm-? I haven't seen a progress update in the last few days and time is almost up.
I hope we haven't lost the battle. I repeatedly fall into the trap of loading messages that won't go away, and the workaround - shut down and restart mail - is very painful.
Give us one more day. Scott, any progress? I'll take a look today.
Looks like the message pane is still being allowed to change its own name. I've reproduced this in Mozilla with mail JS activated. As Scott suggested, it wasn't showing up in Mozilla because mail JS was off by default. Looks like we'll just have to add a check somewhere that sats self.name is read-only if you're a message pane. While we're at it, is there anything else the message pane shouldn't be able to do? I will try to have a fix for this tomorrow (Fri), can we get it into rtm?
If you get me a patch, I'll try to get it through PDT =).
mstoltz, any luck with a patch?
If you can get me a patch today I'll take it to PDT while they are meeting. Thanks!
Not sure I'm going to get to this.
I was thinking about just preventing scripts in mail from setting self.name, but is there a larger issue here? Are there any other, similar things whoich will break mail?
On the surface, self.name is the only one I can think of. Actually, should we prevent it from bringing up dialogs or is that already blocked.
Changing summary to include the JS security concern. Please attach the patch if you can. The approval step is predicated on it existing.
I'd still like to get a fix for this Mitch, if you think it'll be easy. If you don't have time to get to it, if you can point me in the right direction then maybe I can figure it out.
I'm working on a fix tonight. I think I can modify the security manager to prevent mail messages from writing to window.name.
I have a fix, testing to make sure it's safe...
Created attachment 17537 [details] [diff] [review] Patch - allow per-scheme sec policies and add policy for mail
Mitch does mOrigin come from a URI? If so, we can just call ->GetScheme() to get the scheme instead of trying to parse the scheme out of the string ourselves.
mOrigin is a string originally read out of prefs; it can be either an URL like http://warp or just a scheme, like http:. Basically all the operations are on strings; it would add more complexity to build URLs and then parse out the scheme, I think.
note to mscott: following varada's steps to reproduce, I was able to reproduce this bug. then I applied mstoltz's patch and rebuilt, and the problem is fixed.
Mitch, can you get a reviewer from someone in your group to review your patch? I can be the super reviewer for this. thanks for trying this out Seth. sr=mscott It'd be great if we could get the review done today so we can take it to PDT. Thanks again for fixing this for us!
sorry for the extra email. Removing mail2 keyword.
Mitch, would it be possible to get another security review so we can try to get this in this weekend before Monday's build?
Mitch, I'd really like to get this checked in today but we need another reviewer from soneone on your team that's familiar with this code. Who can we pester?
beard has reviewed this. Marking rtm+ to expedite pdt review.
checked into the branch and the tip. thanks again mitch.
Verified as fixed on branch build of win32, linux, and macos using the following builds: win32 commercial seamonkey build 2000-102409-mn6 installed on P500 Win98 linux commercial seamonkey build 2000-102409-mn6 installed on P200 RedHat 6.2 macos commercial seamonkey build 2000-102308-mn6 installed on G3/400 OS 9.04
Win32 (2001-07-10-05-0.9.2) This bug is gone.