644993 - Undo close tab doesn't refresh the content in a SSL Error page

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect, P2)

Description

[Anna (Waverley)]

Build Id:Mozilla /5.0 (Android;Linux armv7l;rv:2.0b13pre) Gecko/20110325 Firefox/4.0b13pre Fennec /4.1a1pre 

Device: Motorola Droid 2 (Android 2.2)

Steps to reproduce:
1. Open a new tab.
2. Visit https://overstock.com (you should see a SSL Error page) 
3. Slide to the left sidebar 
4. Close the tab (hit the X icon)
5. Click the undo closed tab thumbnail at the bottom of the tab sidebar

Actual results:
After step 4, the tab reappears in the active tab sidebar, but doesn't refresh the content in the page. The tab will show a blank thumbnail and a blank webpage.

Expected Results: 
After step 4, the tab will reappear in the active tab sidebar and refresh the content in the page.


Note: I also noticed something else:

Steps to reproduce:
1. Load www.google.com
2. Slide to the left side bar (-> notice that the tab will show a thumbnail of www.google.com)
3. Load https://overstock.com (you should see a SSL Error page) in the same tab. 
4. Slide to the left side bar again.

Actual results:
After step 4, the tab will show a thumbnail of www.google.com

Expected results:
After step 4, the tab should show a thumbnail of https://overstock.com

Are these two issues related?

Comment 1

[Kevin Brosnan [Ex-Mozilla]]

Nominating for post 4.0.

Comment 2

[Lucas Rocha (:lucasr)]

I'm getting slightly different results. On the first series of steps, what I get is the SSL error page with a tab thumbnail for that page. I don't get a blank page with blank thumbnail.

Apparently, when you first access the page that triggers the SSL error page, the thumbnail is not updated at all. This is more or less expected as the browser is waiting for user input on the certificate before actually accessing the page. But I'd still expect the thumbnail to show the SSL error page, not the previous page. When you undo close tab though, the thumbnail is updated to show the SSL error page.

I get the same results on the second series of steps.

So, it seems to be that the actual remaining bug here is that the thumbnail is not consistently updated to show the SSL error page as you'd expect to. 

Investigating...

Comment 3

[Lucas Rocha (:lucasr)]

Attachment: Force thumbnail updates when error pages are shown in browser.

Notify browser that an error page is being shown instead of the target location and force a thumbnail refresh accordingly. This is necessary because the browser contentWindowId is not updated when those error pages are shown.

Comment 4

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 548566 [details] [diff] [review]
Force thumbnail updates when error pages are shown in browser.


>     // Do not repaint thumbnail if we already painted for this load. Bad things
>     // happen when we do async canvas draws in quick succession.
>-    if (!browser || this._thumbnailWindowId == browser.contentWindowId)
>+    if (!forceUpdate && (!browser || this._thumbnailWindowId == browser.contentWindowId))
>       return;

If "pageshow" is fired before the NetworkStop callback is fired, then we could check this in the above function:

  let forceUpdate = (browser && browser.currentURI.spec != browser.documentURI.spec);
  if (!forceUpdate && (!browser || this._thumbnailWindowId == browser.contentWindowId))

Comment 5

[Lucas Rocha (:lucasr)]

(In reply to comment #4)
> Comment on attachment 548566 [details] [diff] [review] [review]
> Force thumbnail updates when error pages are shown in browser.
> 
> 
> >     // Do not repaint thumbnail if we already painted for this load. Bad things
> >     // happen when we do async canvas draws in quick succession.
> >-    if (!browser || this._thumbnailWindowId == browser.contentWindowId)
> >+    if (!forceUpdate && (!browser || this._thumbnailWindowId == browser.contentWindowId))
> >       return;
> 
> If "pageshow" is fired before the NetworkStop callback is fired, then we
> could check this in the above function:
> 
>   let forceUpdate = (browser && browser.currentURI.spec !=
> browser.documentURI.spec);
>   if (!forceUpdate && (!browser || this._thumbnailWindowId ==
> browser.contentWindowId))

I tried that but what actually happens is that by the time updateThumbnail is called, browser.currentURI.spec is the target URL (which will trigger the SSL error, for example) and browser.documentURI.spec still points to the previous URL. The only way I found to catch the proper documentURI pointing to the error page was on DOMContentLoaded/pageshow event on the content side.

Maybe there's another way to query the documentURI here that would work as expected? Dunno...

Comment 6

[Lucas Rocha (:lucasr)]

Attachment: Updated patch with hg user info and commit message

Comment 7

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 549864 [details] [diff] [review]
Updated patch with hg user info and commit message

After thinking this overall for a bit, I agree with the approach. I wanted to avoid sending a new message if we could reuse an existing message. However, only "pageshow" in the <browser> content script makes sense (we could have added a property to the JSON data), but this bug is application specific, not toolkit specific. So content.js makes more sense.

However, you add the handler to ViewportHandler because it had a "pageshow" listener, but an object that handles Viewport behavior should not handle error page behavior.

We do have Content though, which does handle error page behavior. Please add a "pageshow" listener and move the errorpage check into Content.

Put the new "pageshow" near the existing "pagehide":
http://mxr.mozilla.org/mozilla-central/source/mobile/chrome/content/content.js#282

r+ once you make the change, but put up a new patch please.

Comment 8

[Lucas Rocha (:lucasr)]

Attachment: Updated patch to move ErrorPage-handling to Content

'pageshow' is not emitted when error page is shown. Using DOMContentLoaded instead.

Comment 9

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 552658 [details] [diff] [review]
Updated patch to move ErrorPage-handling to Content

>+  updateThumbnail: function updateThumbnail(options) {
 
>+    let forceUpdate = ('force' in options && options.force);

silly nit. My OCD is kicking in: "force"

r+

Comment 10

[Mark Finkle (:mfinkle) (use needinfo?)]

tweaked and pushed:
http://hg.mozilla.org/integration/mozilla-inbound/rev/bd9891ab14eb

Comment 11

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

http://hg.mozilla.org/mozilla-central/rev/bd9891ab14eb

Comment 12

[Andreea Pod]

Verified fixed on build: Mozilla /5.0 (Android;Linux armv7l;rv:8.0a2) Gecko/20110821 Firefox/8.0a2 Fennec/8.0a2
Device: LG Optimus 2X (Android 2.2)

I verified this following the steps form description.