Last Comment Bug 726272 - [Page Thumbnails] don't capture error pages
: [Page Thumbnails] don't capture error pages
Product: Firefox
Classification: Client Software
Component: General (show other bugs)
: Trunk
: All All
-- minor (vote)
: Firefox 14
Assigned To: Tim Taubert [:ttaubert]
: 736054 (view as bug list)
Depends on: 805338
  Show dependency treegraph
Reported: 2012-02-11 00:33 PST by Chris Peterson [:cpeterson]
Modified: 2012-10-24 22:11 PDT (History)
7 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

patch v1 (WIP) (988 bytes, patch)
2012-03-15 07:42 PDT, Tim Taubert [:ttaubert]
no flags Details | Diff | Splinter Review
patch v2 (2.52 KB, patch)
2012-03-15 08:29 PDT, Tim Taubert [:ttaubert]
dietrich: review+
akeybl: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description User image Chris Peterson [:cpeterson] 2012-02-11 00:33:51 PST
One of my New Tab thumbnails (which happens to be is labeled "Problem loading page" (Firefox's "server not found" error page).

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 1 User image Tim Taubert [:ttaubert] 2012-03-15 07:39:27 PDT
*** Bug 736054 has been marked as a duplicate of this bug. ***
Comment 2 User image Tim Taubert [:ttaubert] 2012-03-15 07:42:44 PDT
Created attachment 606209 [details] [diff] [review]
patch v1 (WIP)

Needs a testcase.
Comment 3 User image Tim Taubert [:ttaubert] 2012-03-15 08:29:04 PDT
Created attachment 606226 [details] [diff] [review]
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 User image :Gavin Sharp [email:] 2012-03-15 11:00:59 PDT
Can you instead check channel instanceof nsIHttpChannel && channel.requestSucceeded ?
Comment 5 User image Dietrich Ayala (:dietrich) 2012-03-15 14:44:25 PDT
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 User image Tim Taubert [:ttaubert] 2012-03-16 03:30:40 PDT
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 User image Dietrich Ayala (:dietrich) 2012-03-16 07:42:19 PDT
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 User image Tim Taubert [:ttaubert] 2012-03-16 08:16:36 PDT
Comment 10 User image Tim Taubert [:ttaubert] 2012-03-17 01:28:31 PDT
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 User image Siddhartha Dugar [:sdrocking] 2012-03-17 23:10:57 PDT
This makes it impossible to have thumbnails for pages like about:config (manually pinned). Is that acceptable?
Comment 12 User image Tim Taubert [:ttaubert] 2012-03-18 00:44:16 PDT
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 User image Siddhartha Dugar [:sdrocking] 2012-03-18 00:49:39 PDT
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 User image Alex Keybl [:akeybl] 2012-03-20 13:29:48 PDT
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 User image Tim Taubert [:ttaubert] 2012-03-22 01:57:58 PDT
Comment 16 User image Paul Silaghi, QA [:pauly] 2012-05-16 08:00:24 PDT
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

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