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)
Core
DOM: Navigation
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)
1.99 KB,
patch
|
dagger.bugzilla
:
review+
|
Details | Diff | Splinter Review |
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).
Proposed trivial patch
Attachment #744590 -
Flags: review?(bzbarsky)
Comment 2•11 years ago
|
||
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.
Comment 5•11 years ago
|
||
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.
Comment 7•11 years ago
|
||
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.
Comment 9•11 years ago
|
||
> 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?
Comment 10•11 years ago
|
||
Well, ideally the UI would not clear the url bar on error page loads like this...
Flags: needinfo?(gavin.sharp)
Reporter | ||
Comment 11•11 years ago
|
||
Gavin?
Assignee | ||
Comment 12•11 years ago
|
||
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.
Comment 13•11 years ago
|
||
(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)
Comment 14•11 years ago
|
||
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)
Comment 15•11 years ago
|
||
(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)
Assignee | ||
Comment 16•11 years ago
|
||
Something like this? (Comments are hard.)
Attachment #8338174 -
Flags: review?(dao)
Comment 17•11 years ago
|
||
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+
Assignee | ||
Comment 18•10 years ago
|
||
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
Updated•10 years ago
|
Attachment #744590 -
Attachment is obsolete: true
Comment 19•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/048be1fe0b10
Assignee: nobody → dagger.bugzilla
Keywords: checkin-needed
Comment 20•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/048be1fe0b10
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Updated•10 years ago
|
Whiteboard: [good first verify]
You need to log in
before you can comment on or make changes to this bug.
Description
•