Closed
Bug 337339
Opened 18 years ago
Closed 18 years ago
[FIX]Keyword search pages containing relative urls appear broken
Categories
(Core :: DOM: Navigation, defect, P2)
Core
DOM: Navigation
Tracking
()
RESOLVED
FIXED
mozilla1.9alpha1
People
(Reporter: mossop, Assigned: bzbarsky)
References
Details
(Keywords: fixed1.8.1, regression, relnote)
Attachments
(3 files, 2 obsolete files)
30.42 KB,
patch
|
darin.moz
:
superreview+
|
Details | Diff | Splinter Review |
915 bytes,
patch
|
bzbarsky
:
review+
bzbarsky
:
superreview+
|
Details | Diff | Splinter Review |
32.06 KB,
patch
|
mconnor
:
approval1.8.1+
|
Details | Diff | Splinter Review |
When using keyword searches from the url bar (for failed urls) the page displayed appears broken when relative urls are used. To reproduce: 1. Set keyword.URL to http://users.blueprintit.co.uk/~dave/filestore/keyword/testsearch.html?whatever= 2. Type "testing testing" into the url bar. 3. View the page and see that it has a broken image. Regression range: http://tinderbox.mozilla.org/bonsai/cvsquery.cgi?module=PhoenixTinderbox&date=explicit&mindate=1146743774&maxdate=1146855420 Bug 335457 is about the only likely looking candidate to my eyes.
Assignee | ||
Comment 1•18 years ago
|
||
So the problem is that the HTTP channel the keyword protocol handler creates has the keyword: URI as the originalURI and does NOT have LOAD_REPLACE set. The only way I see offhand to deal with this is to have a separate keyword channel class that would wrap the final channel and add the LOAD_REPLACE flag, then just pass on AsyncOpen. Do we care to QI to nsIHttpChannel and such too, sorta the way view-source does? It's only an issue for the docshell code that creates the channel, and I'd _think_ we don't want referrers and such going with a keyword search, right? Or POST data? Or anything else like that? So I'm tempted to say "no"...
Flags: blocking1.9a1?
Comment 2•18 years ago
|
||
Right, it should not need to implement those interfaces. View source is special because we want to make sure we can view what would be loaded w/ various configurations of the HTTP channel. That said, I'd really like to avoid adding another wrapping nsIChannel implementation if it can be helped. Why is LOAD_REPLACE needed? Why isn't the solution merely to set the OriginalURI property on the channel created by nsKeywordProtocolHandler::NewChannel?
Comment 3•18 years ago
|
||
The nsResProtocolHandler is very similar to nsKeywordProtocolHandler, and all it does is set OriginalURI. Is it also broken? If not, then let's just change the keyword protocol handler to do the same thing.
Comment 4•18 years ago
|
||
I also don't understand why you can't just set the load flag using SetLoadFlags?
Assignee | ||
Comment 5•18 years ago
|
||
The call tree here is:
nsDocShell stuff
NS_NewChannel
ioService->NewChannel();
channel->SetLoadFlags()
channel->SetOriginalURI();
So the docshell will always clobber whatever original URI got set in newChannel. NS_NewChannel will always clobber whatever flags got set in newChannel. Hence setting either one in nsKeywordProtocolHandler::NewChannel is effectively a no-op. I need to set them later.
> Why is LOAD_REPLACE needed?
Because unlike the resource: case (where we want the document to see the original resource: uri as opposed to the final jar: or file: URI), here we want the document to see the final http: URI, not the original keyword: URI.
Comment 6•18 years ago
|
||
OK, I see. I guess I don't understand fully how the system treats LOAD_REPLACE. I still think it is a good idea for nsKeywordProtocolHandler::NewChannel to set the OriginalURI property of the resulting channel for consistency. That way it is clear from the channel that another URI (the OriginalURI) was passed to NewChannel.
Assignee | ||
Comment 7•18 years ago
|
||
The way LOAD_REPLACE is treated is that if it's set then the channel's URI is the "real" URI. If it's not set, then the originalURI is the "real" URI....
Comment 8•18 years ago
|
||
See also bug 264830, "If keyword.URL doesn't lead to a redirect, resulting page has bogus 'keyword:' URL".
Reporter | ||
Comment 10•18 years ago
|
||
The keyword: url remaining in the url bar happened even in Firefox 1.0 from my tests, but the pages it displayed worked correctly.
Assignee | ||
Comment 11•18 years ago
|
||
Right, but that was a bug. ;) Which I fixed in bug 335457. So now we consistently treat that page as having a keyword: URL.... Anyway, I'll try to do comment 1 in the next couple of days.
Updated•18 years ago
|
Component: Location Bar and Autocomplete → Embedding: Docshell
Product: Firefox → Core
QA Contact: location.bar → docshell
Assignee | ||
Comment 12•18 years ago
|
||
Attachment #221872 -
Flags: superreview?(darin)
Attachment #221872 -
Flags: review?(cbiesinger)
Assignee | ||
Updated•18 years ago
|
Assignee: nobody → bzbarsky
OS: Windows XP → All
Priority: -- → P2
Hardware: PC → All
Target Milestone: --- → mozilla1.9alpha
Assignee | ||
Comment 13•18 years ago
|
||
Is there a way I can make the keyword dir not pull too?
Attachment #221872 -
Attachment is obsolete: true
Attachment #221880 -
Flags: superreview?(darin)
Attachment #221880 -
Flags: review?(cbiesinger)
Attachment #221872 -
Flags: superreview?(darin)
Attachment #221872 -
Flags: review?(cbiesinger)
Assignee | ||
Updated•18 years ago
|
Summary: Keyword search pages containing relative urls appear broken → [FIX]Keyword search pages containing relative urls appear broken
Comment 14•18 years ago
|
||
(In reply to comment #13) > Is there a way I can make the keyword dir not pull too? You just have to remove all files (including .cvsignore files) Does anyone use the keyword: protocol directly, who would be broken by this?
Comment 15•18 years ago
|
||
Heh, I frequently type "keyword:" followed by some search terms into my Firefox. That's only because I had to hack off the automatic attempts at keyword searching because they suck lots, though. :) It also has the advantage that they are remembered and kept in the auto-complete list...
Assignee | ||
Comment 16•18 years ago
|
||
> Does anyone use the keyword: protocol directly, who would be broken by this?
There are no consumers in mozilla.org CVS. It's DenyProtocol in CheckLoadURI, so can't be linked to. That leaves people typing it in by hand (like James) and extensions. Frankly, I would say the former is a tiny number and the latter can use the URI fixup API I'm exposing.
Comment 17•18 years ago
|
||
Comment on attachment 221880 [details] [diff] [review] Darin voted that we nuke this altogether OK, users who want this can probably just create a bookmark w/ keyword. nsDefaultURIFixup.cpp MangleKeywordIntoURI is just copied from elsewhere, right? + // if we can't find a keyword.URL keywords won't work. + if (url.IsEmpty()) + return NS_ERROR_FAILURE; NS_ERROR_NOT_AVAILABLE?
Attachment #221880 -
Flags: review?(cbiesinger) → review+
Comment 18•18 years ago
|
||
someone should remove shopKeyword and such from http://lxr.mozilla.org/seamonkey/source/xpfe/browser/resources/locale/en-US/region.properties#3
Assignee | ||
Comment 19•18 years ago
|
||
> MangleKeywordIntoURI is just copied from elsewhere, right?
Yes, from the current keyword: protocol handler.
Attachment #221880 -
Attachment is obsolete: true
Attachment #221989 -
Flags: superreview?(darin)
Attachment #221880 -
Flags: superreview?(darin)
Updated•18 years ago
|
Attachment #221989 -
Flags: superreview?(darin) → superreview+
Assignee | ||
Comment 20•18 years ago
|
||
Fixed. Rest in peace, little protocol handler. Let your dreams be untroubled.
Status: NEW → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
Comment 21•18 years ago
|
||
netwerk/protocol/keyword/still remains in allmakefiles.sh.
Assignee | ||
Comment 22•18 years ago
|
||
Comment on attachment 222720 [details] [diff] [review] remove netwerk/protocol/keyword/ from allmakefiles.sh I just checked this in. In general, though, it's worth it to request review to keep patches from getting lost.
Attachment #222720 -
Flags: superreview+
Attachment #222720 -
Flags: review+
Assignee | ||
Updated•18 years ago
|
Flags: blocking1.9a1?
Comment 23•18 years ago
|
||
Anyone have cycles to get a 1.8 branch suitable version of this? We need bug 335457 for that branch and don't want to take that without a fix for this.
Flags: blocking1.8.1+
Assignee | ||
Comment 24•18 years ago
|
||
Darin, do you think it's OK to remove keyword: on branch? If so, we just need to deal with the nsIURIFixup changes... Otherwise we need a totally different approach.
Comment 25•18 years ago
|
||
Yeah, just remove it! :)
Assignee | ||
Comment 26•18 years ago
|
||
OK. Merging this to branch correctly will be much more painful and time-consuming than I can deal with in the near future because bug 245597 landed on branch, but not the same way it landed on trunk, etc. Easiest might be to just back out that checkin, land this, then land the trunk version of that bug on branch; failing that, the patch here needs surgery. I won't be able to do anything like that until I get back in mid-July at this point, esp. since I'd need to write tests to make sure I'm not regressing bug 245597. Peter, if you have such tests they'd be much appreciated (and should be checked into the tree!). If no one is able to do this before then, I guess I'll look at this when I get back...
Comment 27•18 years ago
|
||
I'm not sure how to write tests for this, but I do have a set of testcases that I test to make sure my patch is working, in the form of typing things into the urlbar and seeing what they do. If anyone can give me a pointer on how to turn those into automated tests I'd be happy to write them. As I mentioned on bug 245597, I'd actually _prefer_ that you back out my branch patch, merge this, and then merge my trunk patch. I don't know how cleanly your patch merges otherwise. If it's clean, someone else with CVS access can get the work done. Presumably the patches that need a+ are the last two listed above?
Assignee | ||
Comment 28•18 years ago
|
||
This is the 1.8 branch version of the patch; I backed out the branch version of bug 337339, merged, and reapplied the trunk version of bug 337339.
Assignee | ||
Updated•18 years ago
|
Attachment #227330 -
Flags: approval1.8.1?
Updated•18 years ago
|
Attachment #227330 -
Flags: approval1.8.1? → approval1.8.1+
Keywords: relnote
You need to log in
before you can comment on or make changes to this bug.
Description
•