948849 - Thumbnail on home page missing for local html file

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Dave Royal]

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

[Aaron Train [:aaronmt]]

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

Comment 2

[Josh Dover [:jdover]]

Attachment: Remove check for successful tab state, allow thumbnail loading exception handling to handle errors.

Comment 3

[Brian Nicholson (: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

Comment 4

[Josh Dover [:jdover]]

Attachment: Forward nsIRequest's success status if not a HTTP request.

Comment 5

[Josh Dover [:jdover]]

(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.

Comment 6

[Brian Nicholson (:bnicholson)]

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

Comment 7

[Josh Dover [:jdover]]

(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

[Brian Nicholson (:bnicholson)]

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.

Comment 9

[Josh Dover [:jdover]]

Attachment: Forward nsIRequest's success status if not a HTTP request.

Comment 10

[Josh Dover [:jdover]]

Attachment: Forward nsIRequest's success status if not a HTTP request.

Comment 11

[Brian Nicholson (:bnicholson)]

Can you add a comment to this before marking checkin-needed (see comment 8)? Without context, the code isn't very obvious.

Comment 12

[Josh Dover [:jdover]]

Attachment: Forward nsIRequest's success status if not a HTTP request.

Comment 13

[Josh Dover [:jdover]]

(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.

Comment 14

[Brian Nicholson (:bnicholson)]

Thanks!

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/63d31c0367e9

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/63d31c0367e9