Closed Bug 337339 Opened 15 years ago Closed 15 years ago

[FIX]Keyword search pages containing relative urls appear broken

Categories

(Core :: DOM: Navigation, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla1.9alpha1

People

(Reporter: mossop, Assigned: bzbarsky)

References

Details

(Keywords: fixed1.8.1, regression, relnote)

Attachments

(3 files, 2 obsolete files)

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.
Blocks: 335457
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?
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?
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.
I also don't understand why you can't just set the load flag using SetLoadFlags?
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.

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.
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....
See also bug 264830, "If keyword.URL doesn't lead to a redirect, resulting page has bogus 'keyword:' URL".
Yeah, that's basically the same issue.
Blocks: 264830
The keyword: url remaining in the url bar happened even in Firefox 1.0 from my tests, but the pages it displayed worked correctly.
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.
Component: Location Bar and Autocomplete → Embedding: Docshell
Product: Firefox → Core
QA Contact: location.bar → docshell
Attached patch Fix (obsolete) — Splinter Review
Attachment #221872 - Flags: superreview?(darin)
Attachment #221872 - Flags: review?(cbiesinger)
Assignee: nobody → bzbarsky
OS: Windows XP → All
Priority: -- → P2
Hardware: PC → All
Target Milestone: --- → mozilla1.9alpha
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)
Summary: Keyword search pages containing relative urls appear broken → [FIX]Keyword search pages containing relative urls appear broken
(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?
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...
> 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 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+
> 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)
Attachment #221989 - Flags: superreview?(darin) → superreview+
Fixed.  Rest in peace, little protocol handler.  Let your dreams be untroubled.
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
netwerk/protocol/keyword/still remains in allmakefiles.sh.
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+
Flags: blocking1.9a1?
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+
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.
Yeah, just remove it! :)
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...
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?
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.
Attachment #227330 - Flags: approval1.8.1?
Attachment #227330 - Flags: approval1.8.1? → approval1.8.1+
Fixed on 1.8.1 branch.
Keywords: fixed1.8.1
Blocks: 130089
You need to log in before you can comment on or make changes to this bug.