Closed Bug 402574 Opened 14 years ago Closed 14 years ago

Identity UI lost when switching between tabs

Categories

(Firefox :: General, defect, P2)

defect

Tracking

()

VERIFIED FIXED
Firefox 3 beta2

People

(Reporter: KaiE, Assigned: KaiE)

References

Details

Attachments

(3 files, 1 obsolete file)

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.
Flags: blocking-firefox3?
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.
Flags: blocking-firefox3? → blocking-firefox3+
Target Milestone: --- → Firefox 3 M10
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.
looks like a PSM bug
Assignee: johnath → kengert
Priority: -- → P2
Blocks: evtracker
Attached patch Patch v1Splinter Review
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.
Attachment #291239 - Flags: review?(rrelyea)
Comment on attachment 291239 [details] [diff] [review]
Patch v1

r+ rrelyea
Attachment #291239 - Flags: review?(rrelyea) → review+
Because of merge conflicts, this is a patch that combines the patch from this bug and attachment 289973 [details] [diff] [review] from bug 405145.
fixed
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Duplicate of this bug: 406650
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.
Status: RESOLVED → VERIFIED
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
Status: VERIFIED → REOPENED
Resolution: FIXED → ---
Attached patch Patch v3 (obsolete) — Splinter Review
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...
Attached patch Patch v4Splinter Review
(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);
Attachment #302853 - Attachment is obsolete: true
Attachment #302854 - Flags: review?(rrelyea)
Comment on attachment 302854 [details] [diff] [review]
Patch v4

r+ = rrelyea
Attachment #302854 - Flags: review?(rrelyea) → review+
fixed
Status: REOPENED → RESOLVED
Closed: 14 years ago14 years ago
Resolution: --- → FIXED
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.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.