Closed
Bug 948849
Opened 11 years ago
Closed 10 years ago
Thumbnail on home page missing for local html file
Categories
(Firefox for Android Graveyard :: General, defect)
Tracking
(firefox26 affected, firefox27 affected, firefox28 affected, firefox29 affected, fennec-)
RESOLVED
FIXED
Firefox 29
People
(Reporter: dave, Assigned: jdover)
References
()
Details
(Keywords: reproducible, testcase, Whiteboard: [mentor=bnicholson][lang=java])
Attachments
(1 file, 4 obsolete files)
1.93 KB,
patch
|
jdover
:
review+
|
Details | Diff | Splinter Review |
User Agent: Mozilla/5.0 (Android; Tablet; rv:26.0) Gecko/26.0 Firefox/26.0 (Nightly/Aurora) Build ID: 20131205073515 Steps to reproduce: Upgraded to 26 Actual results: The first pinned site on the home page is to a local html file. The thumbnail used to display it (at least I'm farly sure it did - I haven't downgraded Fx to check). The thumbnail is now blank apart from the little round globe (?). Expected results: The thumbnail should be displayed. I replaced it with another local htmk page and it wasn't shown either. (Both these pages show a robot in the address bar, which is new.)
Comment 1•11 years ago
|
||
This is reproducible on all channels. Interestingly we get a tab thumbnail in the tab drawer but no thumbnail will appear on about:home. I can reproduce with a simple test-case: data:text/html,testcase
Status: UNCONFIRMED → NEW
status-firefox26:
--- → affected
status-firefox27:
--- → affected
status-firefox28:
--- → affected
status-firefox29:
--- → affected
Ever confirmed: true
Keywords: reproducible,
testcase
Hardware: Other → ARM
Updated•11 years ago
|
tracking-fennec: --- → ?
Updated•11 years ago
|
tracking-fennec: ? → -
Whiteboard: [mentor=bnicholson][lang=java]
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → jdover
Assignee | ||
Comment 2•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Attachment #8356338 -
Flags: review?(bnicholson)
Comment 3•10 years ago
|
||
Comment on attachment 8356338 [details] [diff] [review] Remove check for successful tab state, allow thumbnail loading exception handling to handle errors. Unfortunately, we can't simply remove the state check altogether since this would end up saving thumbnails for about: error pages, 404s, and other HTTP errors. In general, we want to save thumbnails only for "successful" page loads. We usually detect success via nsIHttpChannel [1], but that won't work for local files. So to fix this, we'll want to handle this specific case without breaking our check for all other situations. What may be happening here is that this page load section is not even being entered because this isn't a network request, which we check for at [2] (you'll want to verify this to be sure that's actually the cause). If so, we would need to have an |else if| block that checks for both a) a document stop, and b) nsIHttpChannel.requestSucceeded. Unfortunately, this fix may be more effort than it's worth, since adding these checks here affect *every* state change call and could have a negative performance impact. Just as a next step, can you verify whether this conditional statement at [2] is the reason the thumbnail isn't shown? Note that the Content:StateChange message sent to Java in that block is what's responsible for bubbling the state change to Java and results in the thumbnail being saved. [1] http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#3799 [2] http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#3775
Attachment #8356338 -
Flags: review-
Updated•10 years ago
|
Attachment #8356338 -
Flags: review?(bnicholson)
Assignee | ||
Comment 4•10 years ago
|
||
Assignee | ||
Comment 5•10 years ago
|
||
(In reply to Brian Nicholson (:bnicholson) from comment #3) > Comment on attachment 8356338 [details] [diff] [review] > Remove check for successful tab state, allow thumbnail loading exception > handling to handle errors. > > Unfortunately, we can't simply remove the state check altogether since this > would end up saving thumbnails for about: error pages, 404s, and other HTTP > errors. In general, we want to save thumbnails only for "successful" page > loads. We usually detect success via nsIHttpChannel [1], but that won't work > for local files. So to fix this, we'll want to handle this specific case > without breaking our check for all other situations. > > What may be happening here is that this page load section is not even being > entered because this isn't a network request, which we check for at [2] > (you'll want to verify this to be sure that's actually the cause). If so, we > would need to have an |else if| block that checks for both a) a document > stop, and b) nsIHttpChannel.requestSucceeded. > > Unfortunately, this fix may be more effort than it's worth, since adding > these checks here affect *every* state change call and could have a negative > performance impact. Just as a next step, can you verify whether this > conditional statement at [2] is the reason the thumbnail isn't shown? Note > that the Content:StateChange message sent to Java in that block is what's > responsible for bubbling the state change to Java and results in the > thumbnail being saved. > > [1] > http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/ > browser.js#3799 > [2] > http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/ > browser.js#3775 Worked with Wesley on this, decided that if there was no `nsIHttpChannel` (thrown by an exception), that we would forward along nsIRequest's success status (which is 0 for success) to the Java message. This should succeed for local HTML pages, but still forward the proper failures for 404, 500, etc.
Assignee | ||
Updated•10 years ago
|
Attachment #8356845 -
Flags: review?(bnicholson)
Attachment #8356845 -
Flags: feedback?(mark.finkle)
Comment 6•10 years ago
|
||
Comment on attachment 8356845 [details] [diff] [review] Forward nsIRequest's success status if not a HTTP request. Review of attachment 8356845 [details] [diff] [review]: ----------------------------------------------------------------- So the STATE_IS_NETWORK check at [1] wasn't the issue, and it passes even for local files? That's good news -- this fix works for me then. [1] http://hg.mozilla.org/mozilla-central/file/8f1c9cdedba5/mobile/android/chrome/content/browser.js#l3761
Attachment #8356845 -
Flags: review?(bnicholson) → review+
Assignee | ||
Comment 7•10 years ago
|
||
(In reply to Brian Nicholson (:bnicholson) from comment #6) > Comment on attachment 8356845 [details] [diff] [review] > Forward nsIRequest's success status if not a HTTP request. > > Review of attachment 8356845 [details] [diff] [review]: > ----------------------------------------------------------------- > > So the STATE_IS_NETWORK check at [1] wasn't the issue, and it passes even > for local files? That's good news -- this fix works for me then. > > [1] > http://hg.mozilla.org/mozilla-central/file/8f1c9cdedba5/mobile/android/ > chrome/content/browser.js#l3761 Correct, confirmed to be working 100%, still goes through the STATE_IS_NETWORK state for local files. Thanks for your help.
Comment 8•10 years ago
|
||
Could you also add a comment inside the catch block explaining that the exception will be thrown and 0 status code will be used for local files? You can mention this bug # in the comment if you'd like.
Assignee | ||
Comment 9•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Attachment #8356845 -
Attachment is obsolete: true
Attachment #8356845 -
Flags: feedback?(mark.finkle)
Assignee | ||
Comment 10•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Attachment #8356338 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Attachment #8356854 -
Flags: review+
Assignee | ||
Updated•10 years ago
|
Attachment #8356852 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Updated•10 years ago
|
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 11•10 years ago
|
||
Can you add a comment to this before marking checkin-needed (see comment 8)? Without context, the code isn't very obvious.
Keywords: checkin-needed
Assignee | ||
Comment 12•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Attachment #8356854 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Attachment #8356857 -
Flags: review+
Assignee | ||
Comment 13•10 years ago
|
||
(In reply to Brian Nicholson (:bnicholson) from comment #11) > Can you add a comment to this before marking checkin-needed (see comment 8)? > Without context, the code isn't very obvious. Done.
Keywords: checkin-needed
Comment 14•10 years ago
|
||
Thanks!
Comment 15•10 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/63d31c0367e9
Keywords: checkin-needed
Whiteboard: [mentor=bnicholson][lang=java] → [mentor=bnicholson][lang=java][fixed-in-fx-team]
Comment 16•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/63d31c0367e9
Status: REOPENED → RESOLVED
Closed: 10 years ago → 10 years ago
Resolution: --- → FIXED
Whiteboard: [mentor=bnicholson][lang=java][fixed-in-fx-team] → [mentor=bnicholson][lang=java]
Target Milestone: --- → Firefox 29
Updated•3 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•