Closed Bug 867957 Opened 11 years ago Closed 10 years ago

Location bar became empty if both web search and prefix/suffix addition disabled

Categories

(Core :: DOM: Navigation, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla28

People

(Reporter: mz+bugzilla, Assigned: dagger.bugzilla)

References

Details

(Keywords: regression, Whiteboard: [good first verify])

Attachments

(1 file, 2 obsolete files)

This is regression from Bug 382702, if user disabled both web search and prefix/suffix addition for unknown location, enter some string to location bar and hit enter he get error page (expected) and his location bar became empty (unexpected).
Attached patch Proposed trivial patch (obsolete) — Splinter Review
Proposed trivial patch
Attachment #744590 - Flags: review?(bzbarsky)
Comment on attachment 744590 [details] [diff] [review]
Proposed trivial patch

This doesn't seem right.  Why would about: URIs be special here?
Attachment #744590 - Flags: review?(bzbarsky) → review-
(In reply to Boris Zbarsky (:bz) from comment #2)
> This doesn't seem right.  Why would about: URIs be special here?
It comes from original bug, it should fix only nonexistent "about:" urls, but it also broke other cases. Proposed fix make things as they should be.
OK, so with this patch, what happens if you follow the steps in comment 0?
Following the steps in comment #0, I couldn't reproduce the problem.

Setps(?)
1.
browser.fixup.alternate.enabled;false
keyword.enabled;false
2.
type "about:foo" in the urlbar.


Probably we need another condition or two. Maybe version dependency?

And that code does not erase/overwrite anything. The URI object is NULL because of failure to parse the malformed URI string. Bypassing that block is OK, but you should set a valid URI object to |mFailedURI| instead of "about:blank".
(In reply to Boris Zbarsky (:bz) from comment #4)
> OK, so with this patch, what happens if you follow the steps in comment 0?
After a bit fighting with build process I found that my patch is naive and does not compile with error
error C2664: 'strncmp' : cannot convert parameter 1 from 'const PRUunichar *' to 'const char *'
but I don't know how to properly solve this...

(In reply to O. Atsushi (Torisugari) from comment #5)
> Following the steps in comment #0, I couldn't reproduce the problem.
> 
> Setps(?)
> 1.
> browser.fixup.alternate.enabled;false
> keyword.enabled;false
2. Open new tab, type "simple test string", press Enter
Result: "The address isn't valid" error page appeared and location bar became empty.
Also, if you enter same string in already opened tab, location bar doesn't became empty.
Thanks, now I can reproduce it.

> Result: "The address isn't valid" error page appeared and location bar
> became empty.
> Also, if you enter same string in already opened tab, location bar doesn't
> became empty.

Well, that's exactly what I intended.

Mozilla's session history entry requires always valid URI so as to canonize URI strings pointing to the same document. So if we are to fix this bug in a *right* way, we need another nsIURI implementation which accepts all null-terminated strings, including "simple test string". This is kinda scary thing. Empty "scheme" implies there's no protocol handler. docShell must substitute the *URI* to the nsUltematelySimpleURI::spec.

Another idea is mocking up a new "about:" domain, e.g. "about:malformeduri". If docshell pass "about:malformeduri?mistype=simple test string" to onLocationChane(...), awesomebar will (probably) be able to cut off "about:malformeduri?mistype=" on displaying it. This plan is by far more realistic than the former.

The biggest problem is this bug is a little hard to reproduce for most of users. I agree this is a kind of dataloss, but it's not very severe, is it?
(In reply to O. Atsushi (Torisugari) from comment #7)
> Mozilla's session history entry requires always valid URI so as to canonize
> URI strings pointing to the same document. So if we are to fix this bug in a
> *right* way, we need another nsIURI implementation which accepts all
> null-terminated strings, including "simple test string". This is kinda scary
> thing. Empty "scheme" implies there's no protocol handler. docShell must
> substitute the *URI* to the nsUltematelySimpleURI::spec.
Hm, but then why location bar doesn't became empty if I enter same string again after error? It is inconsistent behavior.

> The biggest problem is this bug is a little hard to reproduce for most of
> users. I agree this is a kind of dataloss, but it's not very severe, is it?
I agree, that on first sight it is not a big problem, but on other hand this counterintuitive behavior may affect other places, forcing user to reenter his input only because it's easier to developers in not looking as right thing.
> Hm, but then why location bar doesn't became empty if I enter same string
> again after error? It is inconsistent behavior.

If I remember correctly, browser.xml has a "input" cache independent from docshell. It (and session restore) uses the cache when you switch the current tab back and forth during writing something in the location bar. And there are complicated conditions when it updates the "input" cache.
see: |useTypedClear| and |userTypedValue|
http://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/browser.xml#644

> I agree, that on first sight it is not a big problem, but on other hand this
> counterintuitive behavior may affect other places, forcing user to reenter
> his input only because it's easier to developers in not looking as right
> thing.

Exactly. This is a valid bug in that sense. What's really problematic is when I create something big to fix a small problem, I often introduce a new bug, in general.


bz, is there any good idea how to handle this bug?
Well, ideally the UI would not clear the url bar on error page loads like this...
Flags: needinfo?(gavin.sharp)
Gavin?
This is actually fallout from bug 623155, it just didn't affect this error page at the time because we used a dialog rather than an error page back then. That bug is marked security sensitive so I can't see the contents, but from the commit:
  http://hg.mozilla.org/mozilla-central/rev/007e6ebef90e
I assume it has something to do with a spoofing risk with SSL certificate error pages.

Would it be reasonable to only clear gBrowser.userTypedValue if aLocation.spec != "about:blank"? It seems SSL errors have aLocation set to the page that triggered the error, and these "URL is not valid" error pages have aLocation set to about:blank. about:blank can't ever trigger an SSL warning, so we won't reintroduce the SSL spoofing issue by doing that.
(In reply to dagger.bugzilla from comment #12)
> This is actually fallout from bug 623155

Thanks for looking into it!

> Would it be reasonable to only clear gBrowser.userTypedValue if
> aLocation.spec != "about:blank"? It seems SSL errors have aLocation set to
> the page that triggered the error, and these "URL is not valid" error pages
> have aLocation set to about:blank. about:blank can't ever trigger an SSL
> warning, so we won't reintroduce the SSL spoofing issue by doing that.

dao, torisugari: what do you think?
Flags: needinfo?(torisugari)
Flags: needinfo?(gavin.sharp)
Flags: needinfo?(dao)
If we were quite sure that a click on a HTML page never triggers such a "Invalid URI" request, that's OK for me. Indeed, no matter what those links' HREF may be, Firefox, as far as I know, always seems to generate a valid URI. I guess that's because the baseURI is probably a valid URI.

However, I don't think we should expect the assumption is always correct. Errors sometimes come via unexpected ways. That's why the strategy "Just do not overwrite the user's input on URLbar" is not very attractive to me.

Docshell (nsIWebNavigation) has the (erroneous) string. And docshell just need a proper way to send it back to the front end.
Flags: needinfo?(torisugari)
(In reply to dagger.bugzilla from comment #12)
> Would it be reasonable to only clear gBrowser.userTypedValue if
> aLocation.spec != "about:blank"? It seems SSL errors have aLocation set to
> the page that triggered the error, and these "URL is not valid" error pages
> have aLocation set to about:blank. about:blank can't ever trigger an SSL
> warning, so we won't reintroduce the SSL spoofing issue by doing that.

My only concern is that this would complicate the check with something where the intent isn't immediately clear, so this should be documented carefully.
Flags: needinfo?(dao)
Something like this? (Comments are hard.)
Attachment #8338174 - Flags: review?(dao)
Comment on attachment 8338174 [details] [diff] [review]
Don't clear urlbar for errors with about:blank as the URI

>+                // If userTypedClear > 0 the document loaded correctly and we should be

add a comma after "If userTypedClear > 0"

>                 if (this.mBrowser.userTypedClear > 0 ||
>-                    (aFlags & Ci.nsIWebProgressListener.LOCATION_CHANGE_ERROR_PAGE))
>+                    ((aFlags & Ci.nsIWebProgressListener.LOCATION_CHANGE_ERROR_PAGE) &&
>+                    aLocation.spec != "about:blank"))

That last line's indentation needs one more space.
Attachment #8338174 - Flags: review?(dao) → review+
Thanks. Updated to address review comments; carrying r+ forward.

This should be ready for checkin, but I can't set checkin-needed on it myself. Could you do that for me?
Attachment #8338174 - Attachment is obsolete: true
Attachment #8340243 - Flags: review+
Keywords: checkin-needed
Attachment #744590 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/048be1fe0b10
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Blocks: 950450
Whiteboard: [good first verify]
You need to log in before you can comment on or make changes to this bug.