Closed Bug 948849 Opened 7 years ago Closed 7 years ago

Thumbnail on home page missing for local html file

Categories

(Firefox for Android :: General, defect)

26 Branch
ARM
Android
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 29
Tracking Status
firefox26 --- affected
firefox27 --- affected
firefox28 --- affected
firefox29 --- affected
fennec - ---

People

(Reporter: dave, Assigned: jdover)

References

()

Details

(Keywords: reproducible, testcase, Whiteboard: [mentor=bnicholson][lang=java])

Attachments

(1 file, 4 obsolete files)

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.)
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
Ever confirmed: true
Hardware: Other → ARM
tracking-fennec: --- → ?
tracking-fennec: ? → -
Whiteboard: [mentor=bnicholson][lang=java]
Assignee: nobody → jdover
Attachment #8356338 - Flags: review?(bnicholson)
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-
Attachment #8356338 - Flags: review?(bnicholson)
(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.
Attachment #8356845 - Flags: review?(bnicholson)
Attachment #8356845 - Flags: feedback?(mark.finkle)
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+
(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.
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.
Attachment #8356845 - Attachment is obsolete: true
Attachment #8356845 - Flags: feedback?(mark.finkle)
Attachment #8356338 - Attachment is obsolete: true
Attachment #8356854 - Flags: review+
Attachment #8356852 - Attachment is obsolete: true
Status: NEW → RESOLVED
Closed: 7 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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
Attachment #8356854 - Attachment is obsolete: true
Attachment #8356857 - Flags: review+
(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
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]
https://hg.mozilla.org/mozilla-central/rev/63d31c0367e9
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
Whiteboard: [mentor=bnicholson][lang=java][fixed-in-fx-team] → [mentor=bnicholson][lang=java]
Target Milestone: --- → Firefox 29
You need to log in before you can comment on or make changes to this bug.