Closed Bug 466527 Opened 16 years ago Closed 15 years ago

Setting the mail start page to non-redirects causes bad display of what's new tab.

Categories

(Thunderbird :: Toolbars and Tabs, defect, P1)

defect

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 3.0b3

People

(Reporter: standard8, Assigned: standard8)

References

Details

(Whiteboard: [b3ux][m6])

Attachments

(1 file, 1 obsolete file)

Steps to reproduce:

1) Set your start page to about:crashes, about:config (or about: anything), or http://ejohn.org/files/border-image/iui/samples/music.html#_home

2) Change the mailnews.start_page_override.mstone to something other than your current setting.

3) (re)Start Thunderbird

Expected Results:

What's new page in a new tab with CSS loaded and looking pretty.

Actual Results:

What's new page in a new tab with no css.

I've taken a brief look, but I'm stumped at the moment.
The page looked the same to me with both the Default start page and about:mozilla after resetting the mstone to 3.0a1pre. Using the 2008/11/24 Windows build.

However, a comparison to the default Start Page had some style differences. The Default had underlined blue text links while the underlines were missing from Whats New blue text links.

View Source was useless to look at the Message display, grayed out. So I tried about:cache, but the Disk cache viewer is not wired up, only the Cache Summary.
I now think I know what causes at least some of this:

- When we load a What's New tab, the iframe we put the data in will be blank, i.e. src = about:blank.

- When a What's New page is referenced directly, e.g. http://www.mozillamessaging.com/en-US/thunderbird/nightly/whatsnew/ our nsMsgContentPolicy comes along and looks up the root docshell [1], in MailShouldLoad [2] we then check the root docshell is http(s):// and then allow loading based on that.

Unfortunately, at this stage the src is about:blank, and hence it fails.

Normally we get away with this - the url is http://live.mozillamessage.com/thunderbird/whatsnew/... that redirects to the normal www page, and hence by that time, src is set to http:// and we load the css and images.


Thinking about it, it still doesn't explain the steps in comment 0, but I wouldn't be surprised if its something very similar.
Summary: Having the start page set to certain pages stops css loading on the what's new page → Setting the what's new page to non-redirects causes css and images not to load
So we are checking the current URL instead of the one we're trying to load?
Some further debug. URLs cut short to aid readability, note that http://live... is a redirect, whereas http://www... is a normal web page.

Loading the start page (in the 3 pane window), request comes from chrome:

ShouldLoad Content Location: http://live.mozillamessaging.com/thunderbird/start
ShouldLoad Requesting Location: chrome://messenger/content/messenger.xul
ShouldLoad Content Location: http://live.mozillamessaging.com/thunderbird/start
ShouldLoad Requesting Location: chrome://messenger/content/messenger.xul

a little later (once the redirect is completed), we get:

ShouldLoad Content Location: http://www.mozillamessaging.com/style/shredder-start.css
ShouldLoad Requesting Location: http://www.mozillamessaging.com/en-US/thunderbird/nightly/start/?uri=/thunderbird/start
MailShouldLoad Content Location: http://www.mozillamessaging.com/style/shredder-start.css
MailShouldLoad Requesting Location: http://www.mozillamessaging.com/en-US/thunderbird/nightly/start/?uri=/thunderbird/start


Loading the whatsnew page seems similar initially:

ShouldLoad Content Location: http://www.mozillamessaging.com/en-US/thunderbird/nightly/whatsnew/
ShouldLoad Requesting Location: chrome://messenger/content/messenger.xul
ShouldLoad Content Location: http://www.mozillamessaging.com/en-US/thunderbird/nightly/whatsnew/
ShouldLoad Requesting Location: chrome://messenger/content/messenger.xul

except we then get:

ShouldLoad Content Location: http://www.mozillamessaging.com/style/shredder-start.css
ShouldLoad Requesting Location: http://www.mozillamessaging.com/en-US/thunderbird/nightly/whatsnew/
MailShouldLoad Content Location: http://www.mozillamessaging.com/style/shredder-start.css
MailShouldLoad Requesting Location: about:blank

nsMsgContentPolicy doesn't like the about:blank, and spits out a warning:

WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80004002: file /Users/moztest/comm/main/src/mailnews/base/src/nsMsgContentPolicy.cpp, line 458


Even if I set the start page to a non-redirect then loading of the start page still works.


If I set the start page to about:config (see comment 0), and set the what's new page to load, for the what's new loading, we get:

ShouldLoad Content Location: http://live.mozillamessaging.com/thunderbird/whatsnew
ShouldLoad Requesting Location: chrome://messenger/content/messenger.xul

followed by:

ShouldLoad Content Location: http://www.mozillamessaging.com/style/shredder-start.css
ShouldLoad Requesting Location: http://www.mozillamessaging.com/en-US/thunderbird/nightly/whatsnew
MailShouldLoad Content Location: http://www.mozillamessaging.com/style/shredder-start.css
MailShouldLoad Requesting Location: about:config


(In reply to comment #3)
> So we are checking the current URL instead of the one we're trying to load?

I think its more correct to say:

Content Location - we are checking the correct URL
Requesting Location - we are attempting to check the root doc shell in the http loading case (in case of multiple nested iframes). If we have a tab which is an iframe (i.e. the what's new page) then the root doc shell appears to equate to the browser element of the 3 pane window which is what I think is causing us the problem.
Added the css inline to the page to fix for now.
Presumably some of the tabmail changes invalidated an assumption made by the code in the else clause starting at:

<http://mxr.mozilla.org/comm-central/source/mailnews/base/src/nsMsgContentPolicy.cpp#348>

I'm going to need to spend some time wrapping my head around how the tabmail stuff works and how it could/should interact with the content policy before contributing more than that.
I have an idea of how to fix this, taking and will make sure its either fixed in b2 or we don't have a problem with b2.
Component: Mail Window Front End → Toolbars and Tabs
Flags: blocking-thunderbird3+
QA Contact: front-end → toolbars-tabs
Summary: Setting the what's new page to non-redirects causes css and images not to load → Setting the mail start page to non-redirects causes bad display of what's new tab.
Whiteboard: [no l10n impact][have an idea on how to fix]
Target Milestone: --- → Thunderbird 3.0b2
Assignee: nobody → bugzilla
Whiteboard: [no l10n impact][have an idea on how to fix] → [no l10n impact][target slushy][have an idea on how to fix]
Attached patch Possible fix (obsolete) — Splinter Review
Here's the potential patch for this, though I'm not convinced by it. Here's some reasoning:

- The what's new iframe is currently a chrome iframe not a content. As we're loading pages we should be loading into content. That's what the js part of this patch fixes.

- The content policy breaks down when it gets to determining if we should load something as it reverts to finding out the URI of the currently loaded message (GetMessagePaneURI).

The issue with this is that it is assuming everything is loading in the message pane, which isn't necessarily true when we have our own separate iframes.

So the patch says that if we haven't got an nsIMsgMailNewsUrl being loaded in the message pane, then we'll use the requesting location to determine if to load the requested page or not.

Whilst it works in this case, I think it may break down if we have a message loaded in the message pane and we attempt to load content in our own iframe.

The only other thought I have just had is could we re-use the message pane - is this the way that we display a message in its own tab? The downside is that we'd be reloading content whenever we switch tabs I believe.

Thoughts?
Attachment #362080 - Flags: superreview?(dmose)
Attachment #362080 - Flags: review?(dmose)
Whiteboard: [no l10n impact][target slushy][have an idea on how to fix] → [no l10n impact][target frozen][needs review/input dmose]
Since this is security-sensitive, and the review is going to require a bunch of thought, we'll do better to do the inline CSS workaround like we did before and move this to beta3, rather than jamming this in hurriedly.
Target Milestone: Thunderbird 3.0b2 → Thunderbird 3.0b3
Whiteboard: [no l10n impact][target frozen][needs review/input dmose] → [no l10n impact][needs review/input dmose]
Status: NEW → ASSIGNED
Whiteboard: [no l10n impact][needs review/input dmose] → [no l10n impact][needs review/input dmose][b3ux]
Comment on attachment 362080 [details] [diff] [review]
Possible fix

This feels fragile to me.  In the interest of improving the code's maintainability, I'd suggest renaming MailShouldLoad to MessagePaneShouldLoad, and restricting it to those semantics. This suggests that there should be something like a CatchAllDocshellShouldLoad method to handle any remaining cases, and that would handle any remaining cases (like, say, random content docshells).  Note that The first clause of MailShouldLoad may want to be factored out into its own method that gets called from multiple place.  This proposal doesn't solve the core problem, but it splits up the various cases in a way that I hope makes them easier to think about.  Thoughts?
Attachment #362080 - Flags: superreview?(dmose)
Attachment #362080 - Flags: superreview-
Attachment #362080 - Flags: review?(dmose)
Attachment #362080 - Flags: review-
Forgot to mention that the JS changes look great.
Whiteboard: [no l10n impact][needs review/input dmose][b3ux] → [no l10n impact][needs new patch][b3ux]
Whiteboard: [no l10n impact][needs new patch][b3ux] → [b3ux][no l10n impact][needs new patch]
This patch is the same as the previous patch without the content policy changes. I've found another bug (bug 374578) that may hold the key to a proper fix for the content policy.

Either way, I think we should be loading what's new into a content iframe, so I'd like to get these js changes in, and follow up with the content policy a little later - I think that any change for the content policy wouldn't require backing out the js changes as both about & what's new should be loaded in a content iframe.

This does also give us the intermediate benefit that links will be displayed with appropriate content styles in the what's new tab.

If you want to test, then you can 

a) change mailnews.start_page_override.mstone to something other than its current value (but don't delete/reset it) - this needs changing for each start up.
b) Set mail.rights.override to false, and the about:rights banner will be displayed every time you start up.
Attachment #362080 - Attachment is obsolete: true
Attachment #369270 - Flags: review?(dmose)
Whiteboard: [b3ux][no l10n impact][needs new patch] → [b3ux][m3][no l10n impact][js changes need review dmose][content policy needs patch]
Depends on: 374578
Whiteboard: [b3ux][m3][no l10n impact][js changes need review dmose][content policy needs patch] → [b3ux][m3][no l10n impact][js changes need review dmose][content policy patch in bug 374578]
Comment on attachment 369270 [details] [diff] [review]
[checked in] Move What's New page to be loaded in content.

Looks good; just a few nits:

>+   * A tab to show content pages.
>+   */
>+  contentTabType: {
>+    frames: 0,

Is "frames" left over from a previous iteration?  I don't see it referenced in this patch, specialTabs.js, or tabmail.xml.  If so, please remove.

>+    closeTab: function onTabClosed (aTab) {
>+    },
>+    saveTabState: function onSaveTabState (aTab) {
>+    },
>+    showTab: function onShowTab (aTab) {
>+    },

Is there any reason that we'd also want an onTitleChanged method?

>+  showWhatsNewPage: function() {

Avoiding anonymous functions makes it easier for debuggers to provide a more useful debugging experience.
Attachment #369270 - Flags: review?(dmose) → review+
Whiteboard: [b3ux][m3][no l10n impact][js changes need review dmose][content policy patch in bug 374578] → [b3ux][m3][no l10n impact][needs updated patch, landing][content policy patch in bug 374578]
Comment on attachment 369270 [details] [diff] [review]
[checked in] Move What's New page to be loaded in content.

Landed with all comments fixed: http://hg.mozilla.org/comm-central/rev/61ce89d965e0
Attachment #369270 - Attachment description: Move What's New page to be loaded in content. → [checked in] Move What's New page to be loaded in content.
Whiteboard: [b3ux][m3][no l10n impact][needs updated patch, landing][content policy patch in bug 374578] → [b3ux][m4]?[fixed apart from content policy patch in bug 374578]
Whiteboard: [b3ux][m4]?[fixed apart from content policy patch in bug 374578] → [b3ux][m5][fixed apart from content policy patch in bug 374578]
Whiteboard: [b3ux][m5][fixed apart from content policy patch in bug 374578] → [b3ux][m6][fixed apart from content policy patch in bug 374578]
Priority: -- → P1
Whiteboard: [b3ux][m6][fixed apart from content policy patch in bug 374578] → [b3ux][m6][just needs bug 374578]
Bug 374578 is now fixed - this bug is fixed by the patch on bug 374578 and the patch already checked in on this bug.
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Whiteboard: [b3ux][m6][just needs bug 374578] → [b3ux][m6]
You need to log in before you can comment on or make changes to this bug.