24274 - Crash in nsWebShell::DoLoadURL

Status: VERIFIED FIXED

(MailNews Core :: Backend, defect, P3)

Description

[Phil Peterson]

1. Run mail
2. Load a folder and a message
3. Click away from the thread pane, causing the thread pane to lose focus.
4. Click back on the selected message in the thread pane
5. Boom.

I've seen this a couple times, but I can't make it happen at will. Scott says
he's seen it, though, so I'm filing it.

Comment 1

[Phil Peterson]

Also, the crash happens at the end of a C++ block scope, so I assume the
destructor of some automatic object (like a COM ptr) is forcing the crash.

Comment 2

[Scott MacGregor]

I finally figured out what was going on here. Marking as M13 as this is a random
crasher when displaying imap messages that's causing problems.
 And the fix is trivial. I'll attach the patch here. Travis, can you give me a
code review on this since Rick Potts is out of town?

DoLoadUrl had code that looked like the following:

nsCOMPtr<nsIChannel> dummyChannel;
rv = NS_OpenURI(getter_AddRefs(dummyChannel), aUri, nsnull, interfaceRequestor);
if (NS_FAILED(rv)) return rv;
if (SOME CONDITION testing if the url has a ref field)
{
	use the dummy channel and return
}
else if (SOME OTHER CONDITION to test if we are loading from session history)
{
   use the dummy channel and return
}

If neither of these conditions were true, then doLoadURL went ahead and kicked
off a regular url load by calling into the uri loader.

The problem was that for a certain case for imap, we were creating a dummy
channel under some bogus conditions such that the imap protocol handler failed
to create a channel and it returned NS_OK back through NS_OpenURI. So the first
fix is to perculate the error back up. Which I did.

However, that's not good enough because of the if (NS_FAILED(rv)) return rv
statement after creating the dummy channel which causes us to never load the url
in this case. It happens to be for imap that neither of the if conditions in my
example above are ever true. We always want to continue forward with the load
even if we failed to create a dummy channel. Because we are never going to use
the dummy channel.

So my fix for webshell::doloadURl was to stall instantiation of the dummy
channel until we are sure we are going to need it. This meant making the call to
NS_OpenURI inside of each if clause.

if (some condition)
{
  create and use dummy channel returning if failure.
}
else if (some condition)
{
  create and use dummy channel, returning if failure.
}

This fixes the problem because now we don't even try to create a dummy channel
if we really aren't going to use it. And the fix is completely safe because we
aren't altering the flow of control at all.

Comment 3

[Scott MacGregor]

Attachment: proposed fix

Comment 4

[Scott MacGregor]

I checked in the fix....

Comment 5

[fenella]

Win32  (2000-04-21-08 M16)
Using the same scenario, I do not see the problem.