402574 - Identity UI lost when switching between tabs

Status: VERIFIED FIXED

(Firefox :: General, defect, P2)

Description

[Kai Engert [:KaiE:]]

Right now you need special preparations to see this bug.

You need a trunk debug build and follow the special instructions from (private/hidden) bug 374336 comment 25 and 26.

Go to a site that triggers the enhanced identification (EV) UI.
(Wait until site has loaded)
Notice there is a company name displayed to the left of the URL bar.
Press CTRL-T to open a new tab.
Use the mouse to switch back to the first tab (which had initially shown the identity area).

Actual behavior:
  Company name is gone, only favicon shown

Expected behavior:
  Company name shown, same display as before.

Comment 1

[Kai Engert [:KaiE:]]

After experiencing the "buggy" display (missing company name), the following is sufficient to get the company name back:

- click in url bar
- hit enter

I'm experiencing this on Linux.

A while ago Johnathan told me he doesn't get this behavior on Mac, so maybe this is platform specific.

Comment 2

[Johnathan Nightingale [:johnath]]

Hey Kai - while I could not reproduce your earlier behaviour in the add-on version of Larry, I can reproduce this on Mac using the instructions provided.  

What I have found is:
- Load EV page
- onSecurityChange fires, and calls checkIdentity on the identity handler ( http://mxr.mozilla.org/mozilla/source/browser/base/content/browser.js#3901 ).
- The state passed in is 1310722 (STATE_IS_SECURE | STATE_SECURE_HIGH | STATE_IDENTITY_EV_TOPLEVEL)
- Everything works as expected.
---
- Switch to another tab and come back
- onSecurityChange fires and calls checkIdentity again, but this time the state passed in is 262146 (STATE_IS_SECURE | STATE_SECURE_HIGH)

I also confirm that clicking the URL bar and hitting enter (triggering a page reload) is sufficient to restore the EV UI, and does send the 1310722 mode in onSecurityChange again.

Kai - It seems that this might be a bug with how webProgressListener/nsSecureBrowserUIImpl decides that something is EV?  Afaict, the browser.js code is behaving correctly for the inputs it's given.

Comment 3

[Mike Connor [:mconnor]]

looks like a PSM bug

Comment 4

[Kai Engert [:KaiE:]]

Attachment: Patch v1

Confirming this as a PSM bug.

The tab-browser called GetState, which is supposed to query the previous security state, but that function did not yet take the EV flag into consideration.

This patch implements the missing piece and fixes the bug in my testing.

Comment 5

[Robert Relyea]

Comment on attachment 291239 [details] [diff] [review]
Patch v1

r+ rrelyea

Comment 6

[Kai Engert [:KaiE:]]

Attachment: Patch v2 for check in

Because of merge conflicts, this is a patch that combines the patch from this bug and attachment 289973 [details] [diff] [review] from bug 405145.

Comment 7

[Kai Engert [:KaiE:]]

fixed

Comment 9

[Tony Chung [:tchung]]

Verified fix on Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9b2) Gecko/2007121014 Firefox/3.0b2.   EV cert appears correctly after tab switching and back.

Comment 10

[Kai Engert [:KaiE:]]

Reopening.

In certain tab switching scenarios this bug is still present.
Example 1:
- start firefox
- load https://bugzilla.mozilla.org
- load https://www.paypal.com
- open new tab
- switch back to first tab
- paypal shown but no longer EV

In addition there is a worse scenario where we report EV although the site is not!
Example 2:
- start firefox
- load https://www.paypal.com
- load https://bugzilla.mozilla.org
- open new tab
- switch back to first tab
- bugzilla now incorrectly shown as EV

Comment 11

[Kai Engert [:KaiE:]]

Attachment: Patch v3

This simple patch fixes the new problem(s).

Note that variables are involved which have confusing names.
I want to rename them:
  mPreviousSecurityState => mNotifiedSecurityState
  mPreviousToplevelWasEV => mNotifiedToplevelIsEV

This patch has the old variable names, to illustrate the minimal fix, but in the following discussion I'm using the new names for mental sanity.

When I implemented the original patch, I followed this thinking: "mNotifiedToplevelIsEV must get updated each time we update mPreviousSecurityState."

That's true, but not sufficient. mNotifiedToplevelIsEV must get updated more often...

The existing buggy code will keep the EV state when the security state does not change. The existing assignments are nested in:
  "if (mNotifiedSecurityState != newSecurityState)"

As a result, our code will notify the most recent (and correct) state.

However, on switching tabs, the UI will query the current state, and we use the content of variables mNotifiedSecurityState / mNotifiedToplevelIsEV, and mNotifiedToplevelIsEV may still contain the EV state of a previous secure site.

The fix is to update mNotifiedToplevelIsEV each time we notify a new toplevel state...

Comment 12

[Kai Engert [:KaiE:]]

Attachment: Patch v4

(Please see the previous patch for the minimal fix.)

This is the fix from patch v3 plus renaming the variables.

I also changed this line to use the mNotified... variables.
They are guaranteed to be identical to the old variables.

-    MapInternalToExternalState(&newState, newSecurityState, mNewToplevelIsEV);
+    MapInternalToExternalState(&newState, mNotifiedSecurityState, mNotifiedToplevelIsEV);

Comment 13

[Robert Relyea]

Comment on attachment 302854 [details] [diff] [review]
Patch v4

r+ = rrelyea

Comment 14

[Kai Engert [:KaiE:]]

fixed

Comment 15

[Tony Chung [:tchung]]

Verified fix on Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9pre) Gecko/2008050704 Minefield/3.0pre.  Kai, if you still repro this, please reopen.