Last Comment Bug 523447 - Limit nsMsgWindow to being an nsIURIContentListener on the message pane docshell only.
: Limit nsMsgWindow to being an nsIURIContentListener on the message pane docsh...
Status: RESOLVED FIXED
[no l10n impact]
: fixed-seamonkey2.0.1
Product: MailNews Core
Classification: Components
Component: Backend (show other bugs)
: Trunk
: All All
-- normal (vote)
: Thunderbird 3.0rc1
Assigned To: Mark Banner (:standard8)
:
:
Mentors:
Depends on:
Blocks: 516776
  Show dependency treegraph
 
Reported: 2009-10-20 13:23 PDT by Mark Banner (:standard8)
Modified: 2009-12-13 06:05 PST (History)
10 users (show)
standard8: blocking‑thunderbird3+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Proposed fix (6.98 KB, patch)
2009-10-20 13:23 PDT, Mark Banner (:standard8)
no flags Details | Diff | Splinter Review
Only apply content listener on message pane. (1.18 KB, patch)
2009-11-02 09:07 PST, Mark Banner (:standard8)
neil: review+
neil: superreview+
Details | Diff | Splinter Review

Description User image Mark Banner (:standard8) 2009-10-20 13:23:29 PDT
Created attachment 407372 [details] [diff] [review]
Proposed fix

Currently, when a mailnews window starts up - ones that typically display messages, but also things like the advanced search - we set the dom window in nsMsgWindow at the same time we hook nsMsgWindow into the top-level docshell of the window as a nsIURIContentListener/parent content listener.

This means that the nsIURIContentListener part of nsMsgWindow will receive notifications for most of the loading that goes on in the window - not just the message pane.

Going back in the cvs history on nsMsgWindow, I find this was originally added in versions 1.6 and 1.8:

http://bonsai.mozilla.org/cvslog.cgi?file=mozilla/mailnews/base/src/nsMsgWindow.cpp&rev=1.8

"The mail window is also a nsIURIContentListener. Implement this interface such that doContent requests are forwarded to the webshell for our message."
"fill out the canHandleContent method to include all the primary and secondary content types we want the mail window to handle"

So when it was originally added, it looks like it was filtering the content to only load certain things. These days the canHandleContent function just forwards directly to the docshell anyway - and it gets that wrong, because it forwards to the messagepane element and not what it is actually being loaded from.

Looking at the remainder of nsIURIContentListener functions, those that do anything special are:

- DoContent() from what I can tell, this is trying to get the channel for the request, and then setting the msgWindow on the URI within the channel.
- IsPreferred() which returns false.

AFAICT DoContent() never succeeds in setting a msgWindow - it never gets that far, I've tried a number of scenarios and I just can't hit that code. Additionally, afaik we're setting all the relevant msgWindows directly on our urls anyway.


Now to the main point of this bug:

IsPreferred() is doing the wrong thing - it always returns false. So if we try and direct content within a browser element (e.g. form submit) then we end up saying we don't want it (not preferred) and pushing it out to the external app handler - default browser. I'd rather just let it determine it based on the browser element we're in.

Whilst we could do an IsPreferred on just the messagepane to set it to false, I'm looking ahead for allowing message loads in different browser elements and having content tabs working correctly. I can't see there would be a problem in emails - we deny javascript, so you're not going to load anything there. If we find there is a case for it, then we can set a content listener on the messagepane docshell and have it check what is loaded in the message pane - if it is a message, then don't allow, otherwise do.


The IsPreferred result is the main thing I want to correct, but I really can't see any reason to have the rest of it either.


My test case on TB was to load clarkbw's google calendar extension, http://hg.mozilla.org/users/clarkbw_gnome.org/googlecalendartab/raw-file/tip/release/googlecalendartab.xpi, set network.protocol-handler.expose-all to true and to try and log in to google calendar using the sign in button - without this patch it would try to open externally, with the patch it will load it within Thunderbird.

I may decide to hold off on landing this for a day or two whilst I check that I do really need it (which I'm pretty sure I do), but want to get it in for reviews to get feedback anyway.
Comment 1 User image Boris Zbarsky [:bz] (still a bit busy) 2009-10-20 13:55:50 PDT
> IsPreferred() is doing the wrong thing - it always returns false.

I suspect that was the whole point of the change, in fact: the goal was to have link clicks in e-mails go and load in the browser, not in the messagepane.  Does your patch not break that?
Comment 2 User image Mark Banner (:standard8) 2009-10-20 14:04:10 PDT
(In reply to comment #1)
> > IsPreferred() is doing the wrong thing - it always returns false.
> 
> I suspect that was the whole point of the change, in fact: the goal was to have
> link clicks in e-mails go and load in the browser, not in the messagepane. 
> Does your patch not break that?

IsPreferred doesn't seem to get called for links (generally in this set up). This appears to be the case whether or not network.protocol-handler.expose-all is true or false (note that false is the normal which means links go to the browser).

The only case I've had it called is when showing the sign in to google calendar page and you click on the "Sign In" button.

It does seem to be that network.protocol-handler.expose-all is the main factor for links in the message/browser elements.
Comment 3 User image Boris Zbarsky [:bz] (still a bit busy) 2009-10-20 14:17:24 PDT
> IsPreferred doesn't seem to get called for links (generally in this set up).

In Thunderbird, or suite (where this code was initially added)?

> note that false is the normal

Not in suite.
Comment 4 User image neil@parkwaycc.co.uk 2009-10-20 14:28:39 PDT
Originally Mozilla relied on the URI content listener to divert all links and form submissions out of the message pane. A click handler was added because clicking on a link to a slow or resource could get cancelled if another message was read while the link was trying to load, although this did cause issues for links to downloads. URI content listeners also cannot divert error pages.

Ideally I wouldn't want arbitrary content to load into a messagepane docshell for at least as long as there's no session history navigation.
Comment 5 User image David :Bienvenu 2009-10-21 08:01:00 PDT
Comment on attachment 407372 [details] [diff] [review]
Proposed fix

I'm going to switch the r/sr on this since Neil is much more aware of the issues and history here.
Comment 6 User image Mark Banner (:standard8) 2009-11-02 06:20:27 PST
I've been doing a bit more work on this. Just changing IsPreferred (either via the patch or changing the return value) isn't enough.

We also have to set network.protocol-handler.expose.http to true as well.

This is because, for forms, we go through nsFormSubmission::SubmitTo -> nsWebShell::OnLinkClickSync. OnLinkClickSync checks with the external protocol handler to see if the protocol is exposed, and if it isn't, it redirects the link click to the external app rather than loading internally.

For Thunderbird to set network.protocol-handler.expose.http to true, this would mean that clicking on links in the message pane would visit those links within the message pane - given that SeaMonkey doesn't do this, there may be something I'm missing.

Investigating a bit more...
Comment 7 User image neil@parkwaycc.co.uk 2009-11-02 07:09:06 PST
SeaMonkey has a click handler which looks for link clicks and diverts them to a browser window. Without that handler, a link click would try to load in the message pane; a form submit still does try, but the URIContentListener prevents it and it ends up loading in a browser window anyway.
Comment 8 User image Mark Banner (:standard8) 2009-11-02 07:28:26 PST
Ok, so SeaMonkey's messagePaneOnClick() function handles the redirect of clicks on http etc to the main browser.

So in theory we could set network.protocol-handler.expose.http to true (and any other appropriate prefs) and have our message pane on click handler redirect clicks to the external app handler service.

The downside to this, is that we'd also need to provide some kind of default handler that toolkit can use - e.g. the links in add-on manager - something that hooks into nsIURILoader - which is an nsIURIContentListener which is what nsMsgWindow provides.

I think there would be ways of moving this forward, but both would probably include limiting the existing registration of the content listener to the message pane docshell rather than the whole window. We may need to ifdef some code as well so that Thunderbird could handle things differently.
Comment 9 User image Mark Banner (:standard8) 2009-11-02 09:07:58 PST
Created attachment 409706 [details] [diff] [review]
Only apply content listener on message pane.

Work in progress patch that's better that the last - with this we still register the content listener but just for the messagepane. This means that we'll do the right thing for messages, but allow other browser elements in the window to do what they want to do.

I think I need more support items/work in here so waiting until I have done a bit more work before requesting review.
Comment 10 User image Mark Banner (:standard8) 2009-11-04 05:59:44 PST
Comment on attachment 409706 [details] [diff] [review]
Only apply content listener on message pane.

Ok, this is the one I can go with. It moves the content listener so it only applies to the message pane docshell and not the whole window. This is sufficient for what I need - hence other browser elements in the mail window will be able to handle forms correctly.

I tested this with a web form in an email and the behaviour was unchanged.

Note You need to log in before you can comment on or make changes to this bug.