726272 - [Page Thumbnails] don't capture error pages

Status: VERIFIED FIXED

(Firefox :: General, defect)

Description

[Chris Peterson [:cpeterson]]

AR:
One of my New Tab thumbnails (which happens to be http://mail.google.com) is labeled "Problem loading page" (Firefox's "server not found" error page).

ER:
The New Tab should not show "Problem loading page" thumbnails. It could substitute the thumbnail's hostname or just omit the thumbnail entirely until a real page title is found.

Comment 2

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v1 (WIP)

Needs a testcase.

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v2

Patch with a test. I accessed gBrowserThumbnails._shouldCapture() directly because it's really hard to tell whether a thumbnails has *not* been captured.

Comment 4

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

Can you instead check channel instanceof nsIHttpChannel && channel.requestSucceeded ?

Comment 5

[Dietrich Ayala (:dietrich)]

Comment on attachment 606226 [details] [diff] [review]
patch v2

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

what gavin said. is a more explicit test that way.

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

So (channel.requestSucceeded == true) and (request.status == 0) because that refers to the internal load of "about:neterror". Should we stick with the old approach or is there any other solution?

Comment 7

[Dietrich Ayala (:dietrich)]

Comment on attachment 606226 [details] [diff] [review]
patch v2

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

old approach is fine for now. i can't think of any other redirect-to-about scenarios, so let's roll with this and update as needed if bugs start flowing in.

Comment 8

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/integration/fx-team/rev/657e7edcb91c

Comment 9

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/657e7edcb91c
https://hg.mozilla.org/mozilla-central/rev/a53f34be3b81

Comment 10

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 606226 [details] [diff] [review]
patch v2

[Approval Request Comment]
Regression caused by (bug #):
User impact if declined: error pages can overwrite thumbnails in cases of network outage
Testing completed (on m-c, etc.): 
Risk to taking this patch (and alternatives if risky): low risk, very small patch with a test
String changes made by this patch: none

Comment 11

[Siddhartha Dugar [:sdrocking]]

This makes it impossible to have thumbnails for pages like about:config (manually pinned). Is that acceptable?

Comment 12

[Tim Taubert [:ttaubert] (inactive)]

We've never taken screenshots of about: pages (this bug is only about error pages). But that doesn't mean we couldn't.

Comment 13

[Siddhartha Dugar [:sdrocking]]

I thought they were missing because of this patch. I was wrong.

The problem happens in Aurora also, which I believe doesn't contain this patch. Please create a followup bug to show thumbnails for about:config and similar pages.

Comment 14

[Alex Keybl [:akeybl]]

Comment on attachment 606226 [details] [diff] [review]
patch v2

[Triage Comment]
Low risk patch in support of correctness in a new feature. Approved for Aurora 13.

Comment 15

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/df303dada20e

Comment 16

[Paul Silaghi, QA [:pauly]]

Page Thumbnails don't capture error pages.
Verified fixed on FF 13b3:
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:13.0) Gecko/20100101 Firefox/13.0
Mozilla/5.0 (X11; Linux i686; rv:13.0) Gecko/20100101 Firefox/13.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:13.0) Gecko/20100101 Firefox/13.0