916667 - about:tabcrashed fails to display on pages without a title

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Mark Hammond [:markh] [:mhammond]]

If a crashing page has no title, nsDocShell::DisplayLoadError fails with NS_ERROR_FAILURE.  This is because the string in the 'crashedPageTitle' is empty, so it attempts to fetch the message 'tabcrashed' from the string bundle, which doesn't exist:

JavaScript error: chrome://browser/content/tabbrowser.xml, line 3124: NS_ERROR_FAILURE: Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIDocShell.displayLoadError]

Comment 1

[Mark Hammond [:markh] [:mhammond]]

Attachment: 0001-Bug-916667-Use-the-page-s-URI-if-no-title-is-availab.patch

This patch uses the URI if no title is available.

Comment 2

[:Gavin Sharp [email: [e-mail]]]

setTabTitle does a few more things:
http://mxr.mozilla.org/mozilla-central/source/browser/base/content/tabbrowser.xml#1187

Should we do those things here too? (factored out into a helper maybe)

Comment 3

[Mark Hammond [:markh] [:mhammond]]

Attachment: 0001-Bug-916667-Refactor-and-reuse-the-logic-in-setTabTit.patch

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #2)
> setTabTitle does a few more things:
> http://mxr.mozilla.org/mozilla-central/source/browser/base/content/
> tabbrowser.xml#1187
> 
> Should we do those things here too? (factored out into a helper maybe)

Yeah, I guess we should.  This patch creates a helper _titleForBrowser(aBrowser) - sadly, it needs to return 2 values - the title itself and a boolean to indicate if the title came from the URI - setTabTitle() cares about the boolean so it can change the 'crop' attribute of the tab if necessary.

The patch has come out a little weird - it looks like I moved 2 unrelated functions, but hopefully it can be untangled :)

Comment 4

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 805812 [details] [diff] [review]
0001-Bug-916667-Refactor-and-reuse-the-logic-in-setTabTit.patch

This should already work, I added a string to the string bundle specifically for this case! see here:

http://mxr.mozilla.org/mozilla-central/source/dom/locales/en-US/chrome/appstrings.properties#37

but.. I only added it to dom/.../appstrings.properties and didn't notice that the browser one overrides it (http://mxr.mozilla.org/mozilla-central/source/browser/locales/en-US/chrome/overrides/appstrings.properties)

Comment 5

[:Felipe Gomes (needinfo for replies!)]

Attachment: Patch

I added the missing string to the browser/ version of appstrings.properties, but turns out that's not enough. I thought that the existence of the string id in the bundle would be enough, but the code in nsDocShell really wants a non-empty string, otherwise it will return an error.

This will make the page set " " as the document title, but the front-end trims it and considers it as a page with no title, and do the right thing (no page title -> display url)

Comment 6

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 806214 [details] [diff] [review]
Patch

Review of attachment 806214 [details] [diff] [review]:
-----------------------------------------------------------------

As discussed on IRC, the addition of the new string wasn't supposed to be included.  r=me with that removed, plus a comment added to nsDocShell saying both that (a) UX wants no title and (b) attempting to add an empty string to the .properties file doesn't do the right thing.

Comment 7

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 806214 [details] [diff] [review]
Patch

Thanks. I'll also write a test for this before landing

Comment 8

[:Felipe Gomes (needinfo for replies!)]

Attachment: Patch w/ test

The test took a frustratingly long time to get right, so it deserves a review. There's also a new check for childMap != null added, which will not happen in a real scenario, but happens during the tests so it's there to avoid the null error msg from poluting the test.

Comment 9

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 808740 [details] [diff] [review]
Patch w/ test

As this adds a new folder for mochitest-chrome tests in browser, it needs a build peer review

Comment 10

[:Felipe Gomes (needinfo for replies!)]

https://tbpl.mozilla.org/?tree=Try&rev=a5016e25326e

Comment 11

[Mike Hommey [:glandium]]

Comment on attachment 808740 [details] [diff] [review]
Patch w/ test

Review of attachment 808740 [details] [diff] [review]:
-----------------------------------------------------------------

for the build parts

Comment 12

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 808740 [details] [diff] [review]
Patch w/ test

Review of attachment 808740 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM!  Ideally we'd also have a test that a crash does actually show the page, but I think that would be fine to have in a followup - it's subtle (as we saw recently with thumbnail crash tests and ASan) and we don't really have the test infrastructure in place for that yet.

::: docshell/base/nsDocShell.cpp
@@ +4347,5 @@
>          nsCOMPtr<Element> element = do_QueryInterface(handler);
>          element->GetAttribute(NS_LITERAL_STRING("crashedPageTitle"), messageStr);
>        }
> +
> +      if (messageStr.IsEmpty()) {

I think it is worth a comment about why we need to do this.

Comment 13

[:Felipe Gomes (needinfo for replies!)]

https://hg.mozilla.org/integration/fx-team/rev/3eb8450ae7d6

Comment 14

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/3eb8450ae7d6