337339 - [FIX]Keyword search pages containing relative urls appear broken

Status: RESOLVED FIXED

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

Description

[Dave Townsend [:mossop]]

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.

Comment 1

[Boris Zbarsky [:bzbarsky]]

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"...

Comment 2

[Darin Fisher]

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

[Darin Fisher]

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

[Christian :Biesinger (don't email me, ping me on IRC)]

I also don't understand why you can't just set the load flag using SetLoadFlags?

Comment 5

[Boris Zbarsky [:bzbarsky]]

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

[Darin Fisher]

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.

Comment 7

[Boris Zbarsky [:bzbarsky]]

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

[Jesse Ruderman]

See also bug 264830, "If keyword.URL doesn't lead to a redirect, resulting page has bogus 'keyword:' URL".

Comment 9

[Boris Zbarsky [:bzbarsky]]

Yeah, that's basically the same issue.

Comment 10

[Dave Townsend [:mossop]]

The keyword: url remaining in the url bar happened even in Firefox 1.0 from my tests, but the pages it displayed worked correctly.

Comment 11

[Boris Zbarsky [:bzbarsky]]

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.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Attachment: Fix

Comment 13

[Boris Zbarsky [:bzbarsky]]

Attachment: Darin voted that we nuke this altogether

Is there a way I can make the keyword dir not pull too?

Comment 14

[Christian :Biesinger (don't email me, ping me on IRC)]

(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

[James Ross]

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...

Comment 16

[Boris Zbarsky [:bzbarsky]]

> 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

[Christian :Biesinger (don't email me, ping me on IRC)]

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?

Comment 18

[Christian :Biesinger (don't email me, ping me on IRC)]

someone should remove shopKeyword and such from http://lxr.mozilla.org/seamonkey/source/xpfe/browser/resources/locale/en-US/region.properties#3

Comment 19

[Boris Zbarsky [:bzbarsky]]

Attachment: Updated to biesi's comments

> MangleKeywordIntoURI is just copied from elsewhere, right?

Yes, from the current keyword: protocol handler.

Comment 20

[Boris Zbarsky [:bzbarsky]]

Fixed.  Rest in peace, little protocol handler.  Let your dreams be untroubled.

Comment 21

[Hiro]

Attachment: remove netwerk/protocol/keyword/ from allmakefiles.sh

netwerk/protocol/keyword/still remains in allmakefiles.sh.

Comment 22

[Boris Zbarsky [:bzbarsky]]

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.

Comment 23

[Mike Schroepfer]

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.   

Comment 24

[Boris Zbarsky [:bzbarsky]]

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

[Darin Fisher]

Yeah, just remove it! :)

Comment 26

[Boris Zbarsky [:bzbarsky]]

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

[Peter Kasting]

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?

Comment 28

[Boris Zbarsky [:bzbarsky]]

Attachment: Merged to 1.8 branch

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.

Comment 29

[Boris Zbarsky [:bzbarsky]]

Fixed on 1.8.1 branch.