Closed Bug 1129564 Opened 5 years ago Closed 5 years ago

[e10s] anchor navigation from the awesomebar removes the lock icon

Categories

(Core Graveyard :: Security: UI, defect)

x86_64
All
defect
Not set

Tracking

(e10sm6+, firefox40 fixed)

RESOLVED FIXED
mozilla40
Tracking Status
e10s m6+ ---
firefox40 --- fixed

People

(Reporter: keeler, Assigned: Felipe)

References

Details

Attachments

(2 files, 1 obsolete file)

STR:
Navigate to an https:// url with an anchor/hash/fragment (e.g. https://dxr.mozilla.org/mozilla-central/source/security/manager/ssl/src/SSLServerCertVerification.cpp#20 )
Click in the awesomebar and hit enter/return/go.
Note that the lock icon in the awesomebar has disappeared.
tracking-e10s: --- → ?
OS: Linux → All
Summary: anchor navigation from the awesomebar removes the lock icon → [e10s] anchor navigation from the awesomebar removes the lock icon
At least, this bug exist since a very old Nightly 25.0a1(2014-Jan-14)
Component: General → Security: UI
Product: Firefox → Core
Version: unspecified → Trunk
Assignee: nobody → mrbkap
Flags: firefox-backlog+
Points: --- → 2
Flags: qe-verify-
Assignee: mrbkap → felipc
Status: NEW → ASSIGNED
Iteration: --- → 39.2 - 23 Mar
Blocks: 1140129
Iteration: 39.2 - 23 Mar → 39.3 - 30 Mar
Iteration: 39.3 - 30 Mar → 40.1 - 13 Apr
Okaay, so this is going to be a fun one, as it involves the dreaded userTypedValue/userTypedClear.

The reason this happens is that, after performing the steps in comment 0, the UI is thinking that the URLBar value is still a user typed value, and thus displays the generic icon instead of the lock icon.

The root of the issue is that the code responsible to clear the userTypedValue after a successful load (in this case, anchor navigation), is tabbrowser.xml's mTabProgressListener.onLocationChange [1].

The difference between e10s and non-e10s is that, when browser.js _loadURIWithFlags calls loadURIWithOptions [2], onLocationChange runs synchronously in non-e10s, but async with e10s.


It goes like this:

non-e10s:
 - browser.js _loadURIWithFlags enters and sets userTypedClear to 1
 - browser.webNavigation.loadURIWithOptions is called
 - onLocationChange is called at this moment and clears userTypedValue (because userTypedClear > 0)
 - URLBarSetURI is called, which expects userTypedValue to be null to have the correct behavior
 - at the end, _loadURIWithFlags sets userTypedClear back to 0


e10s:
 - browser.js _loadURIWithFlags enters and sets userTypedClear to 1
 - browser.webNavigation.loadURIWithOptions is called
 - _loadURIWithFlags continues to run and reach its end, setting userTypedClear to 0
 - later on, onLocationChange is called but since userTypedClear is not > 0, it doesn't clear userTypedValue.
 - URLBarSetURI is called and think there's something the user typed, removing the lock icon


I don't know yet how to fix this situation.. I'll needinfo some people who might have suggestions.


[1] http://mxr.mozilla.org/mozilla-central/source/browser/base/content/tabbrowser.xml?rev=0160303649ae#788
[2] http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js?rev=0160303649ae#853
Flags: needinfo?(gijskruitbosch+bugs)
Flags: needinfo?(dtownsend)
Flags: needinfo?(dao)
We seem to already have e10s-specific code to actually load the page. I'm assuming the e10s behaviour for non-remote URLs is the same as the non-e10s behaviour.

Seems like we could make the finally clause that decrements userTypedClear conditional on the non-remote case, and make onLocationChange decrement it in the remote case?

This has the downside that there is some unknown time where userTypedClear will remain > 0, and on top of that, it might affect what happens for further user modifications to the urlbar value. I don't know this code well enough to know what the ramifications of that would be. I just know that the urlbar-related code is already buggy, cf. bug 998606 and dupes. :-\

I'm hoping Dão can clarify the ramifications of my suggestion further.
Flags: needinfo?(gijskruitbosch+bugs)
No longer blocks: 1140129
userTypedValue/userTypedClear are pretty much mysteries to me so I don't think I have a lot to say here without spending time digging into the code,
Flags: needinfo?(dtownsend)
Iteration: 40.1 - 13 Apr → 40.2 - 27 Apr
Flags: needinfo?(dao)
Attached patch Patch (obsolete) — Splinter Review
So, after much pondering, I think I've arrived on the simplest patch that I'm happy with.

I was going with the route of making the change conditional to the remote case. But it looks like in the end the two cases would just be the same. What matters is that the decrement happens after OnLocationChange runs, so we can move it unconditionally to the end of OnLocationChange.  Note that OnLocationChange already clears userTypedClear/Value earlier, in the first `if (topLevel)` branch, and this is unlikely to matter for non-topLevel, so it seems that the only reason this finally {} clause existed was to cover the rare case where something could go wrong and throw an exception and leave the value wrong.

What you think, Gijs? My main concern here is that this theoretically changes the non-e10s case, and that might go untested for a long period, given that e10s will likely be uplifted to aurora. If that's a big deal, we could uplift this patch to aurora now during this cycle, or go with the safer route and condition this change to the e10s case.

I documented this mechanism better in order to shed some light into userTypedValue, and also to keep the contract clear between the increment in browser.js and the decrement in tabbrowser.
Attachment #8595130 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8595130 [details] [diff] [review]
Patch

Review of attachment 8595130 [details] [diff] [review]:
-----------------------------------------------------------------

This looks OK to me but I would be happier if Dão looked at it as well. I am not enough of an expert on how this code works to be 100% confident.

Also, you trypushed this, right? :-)

It'd also be good if we had tests for this... ideally running for both e10s and non-e10s.

Regarding the aurora situation, I would expect a lot of people to turn off e10s if we do uplift it. I certainly do this all the time, because it still breaks way too much stuff (like, say, running at all on WinXP 64-bit, for the last 2 weeks...).
Attachment #8595130 - Flags: review?(gijskruitbosch+bugs)
Attachment #8595130 - Flags: review?(dao)
Attachment #8595130 - Flags: review+
(in other words, I would just land on Nightly and not uplift)
Comment on attachment 8595130 [details] [diff] [review]
Patch

>--- a/browser/base/content/browser.js
>+++ b/browser/base/content/browser.js
>@@ -842,16 +842,22 @@ function _loadURIWithFlags(browser, uri,

>   if (!(flags & browser.webNavigation.LOAD_FLAGS_FROM_EXTERNAL)) {
>+    // Temporarily raise the userTypedClear to allow the URL bar to be
>+    // changed to the to-be URL while it's not fully loaded yet.
>+    // This is done because the URL bar will only display a custom value
>+    // (that is not the actual url) while this field is greater than 0.
>+    // After the load finishes, tabbrowser's onLocationChange is responsible
>+    // for reverting this increment.
>     browser.userTypedClear++;
>   }

This seems somewhat troubling, because _loadURIWithFlags is a fork of toolkit/content/widgets/browser.xml's loadURIWithFlags, which I hope is supposed to be temporary? browser.xml can't depend on tabbrowser.xml, though, so this will make it harder to unfork this code.

>--- a/browser/base/content/tabbrowser.xml
>+++ b/browser/base/content/tabbrowser.xml
>@@ -828,16 +828,24 @@
>                                             [aWebProgress, aRequest, aLocation,
>                                              aFlags]);
>               }
> 
>               if (topLevel) {
>                 this.mBrowser.lastURI = aLocation;
>                 this.mBrowser.lastLocationChange = Date.now();
>               }
>+
>+              if (this.mBrowser.userTypedClear > 0) {
>+                // browser.js _loadURIWithFlags might have temporarily raised this
>+                // value to allow the URL bar to display the loading URL. As the load
>+                // has completed, we must clear this value if it still wasn't cleared
>+                // above or in any of the other onLocationChange callers.
>+                this.mBrowser.userTypedClear--;
>+              }

Doing this for the !topLevel case seems quite wrong to me, as those location changes have nothing to do with the location bar.

(In reply to :Felipe Gomes from comment #5)
> What matters is that the decrement happens after OnLocationChange runs, so
> we can move it unconditionally to the end of OnLocationChange.

That might be true for anchor navigation. It's not true for other loads, is it? So in those cases userTypedClear would be decremented much later, or not at all if we end up not changing the location; not sure if that's a problem.
Attachment #8595130 - Flags: review?(dao) → review-
(In reply to Dão Gottwald [:dao] from comment #8) 
> This seems somewhat troubling, because _loadURIWithFlags is a fork of
> toolkit/content/widgets/browser.xml's loadURIWithFlags, which I hope is
> supposed to be temporary? browser.xml can't depend on tabbrowser.xml,
> though, so this will make it harder to unfork this code.

browser.js version seems like now just a more complete version of the browser.xml one. I imagine that when we can remove the `gMultiProcessBrowser` condition, unforking it will just mean moving what we have now in browser.js back to browser.xml
Attached patch Alternative 1Splinter Review
Probably not what we want, but it would fix the e10s issue temporarily until a better solution is found.

I'll attach different one which I think is better in a moment
Attachment #8595130 - Attachment is obsolete: true
Attachment #8596590 - Flags: review?(dao)
Attached patch Alternative 2Splinter Review
I guess the correct answer is to just say that f the OnLocationChange happened for an anchor navigation, we need to clear the userTypedValue after OnLocationChange.

One question that I have is if the purpose of the userTypedClear++ in loadURIWithFlags is only to handle anchor navigation, or if it's there for more cases too. If it's only for anchor navigation then we could remove that and just let it be covered by the addition from this patch.
Attachment #8596596 - Flags: review?(dao)
(In reply to :Felipe Gomes from comment #11)
> One question that I have is if the purpose of the userTypedClear++ in
> loadURIWithFlags is only to handle anchor navigation, or if it's there for
> more cases too.

I have no idea. Have you tried to find the original commit adding that code?
Attachment #8596596 - Flags: review?(dao) → review+
Attachment #8596590 - Flags: review?(dao)
https://hg.mozilla.org/mozilla-central/rev/806de9b02d86
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Depends on: 1199934
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.