Last Comment Bug 337339 - [FIX]Keyword search pages containing relative urls appear broken
: [FIX]Keyword search pages containing relative urls appear broken
Status: RESOLVED FIXED
: fixed1.8.1, regression, relnote
Product: Core
Classification: Components
Component: Document Navigation (show other bugs)
: Trunk
: All All
: P2 normal (vote)
: mozilla1.9alpha1
Assigned To: Boris Zbarsky [:bz] (still a bit busy)
:
: Andrew Overholt [:overholt]
Mentors:
Depends on:
Blocks: 130089 264830 335457
  Show dependency treegraph
 
Reported: 2006-05-09 13:32 PDT by Dave Townsend [:mossop]
Modified: 2007-07-09 07:41 PDT (History)
11 users (show)
mtschrep: blocking1.8.1+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Fix (11.21 KB, patch)
2006-05-12 17:17 PDT, Boris Zbarsky [:bz] (still a bit busy)
no flags Details | Diff | Splinter Review
Darin voted that we nuke this altogether (28.51 KB, patch)
2006-05-12 19:17 PDT, Boris Zbarsky [:bz] (still a bit busy)
cbiesinger: review+
Details | Diff | Splinter Review
Updated to biesi's comments (30.42 KB, patch)
2006-05-14 15:21 PDT, Boris Zbarsky [:bz] (still a bit busy)
darin.moz: superreview+
Details | Diff | Splinter Review
remove netwerk/protocol/keyword/ from allmakefiles.sh (915 bytes, patch)
2006-05-20 07:34 PDT, Hiro
bzbarsky: review+
bzbarsky: superreview+
Details | Diff | Splinter Review
Merged to 1.8 branch (32.06 KB, patch)
2006-06-27 18:08 PDT, Boris Zbarsky [:bz] (still a bit busy)
mconnor: approval1.8.1+
Details | Diff | Splinter Review

Description Dave Townsend [:mossop] 2006-05-09 13:32:54 PDT
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 [:bz] (still a bit busy) 2006-05-09 15:28:15 PDT
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 2006-05-09 15:52:44 PDT
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 2006-05-09 15:53:38 PDT
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) 2006-05-09 15:54:37 PDT
I also don't understand why you can't just set the load flag using SetLoadFlags?
Comment 5 Boris Zbarsky [:bz] (still a bit busy) 2006-05-09 16:02:13 PDT
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 2006-05-09 16:28:47 PDT
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 [:bz] (still a bit busy) 2006-05-09 16:35:46 PDT
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 2006-05-09 19:14:31 PDT
See also bug 264830, "If keyword.URL doesn't lead to a redirect, resulting page has bogus 'keyword:' URL".
Comment 9 Boris Zbarsky [:bz] (still a bit busy) 2006-05-09 19:26:43 PDT
Yeah, that's basically the same issue.
Comment 10 Dave Townsend [:mossop] 2006-05-10 03:20:17 PDT
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 [:bz] (still a bit busy) 2006-05-10 07:59:49 PDT
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 [:bz] (still a bit busy) 2006-05-12 17:17:20 PDT
Created attachment 221872 [details] [diff] [review]
Fix
Comment 13 Boris Zbarsky [:bz] (still a bit busy) 2006-05-12 19:17:43 PDT
Created attachment 221880 [details] [diff] [review]
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) 2006-05-13 05:32:44 PDT
(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 2006-05-13 06:11:11 PDT
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 [:bz] (still a bit busy) 2006-05-13 11:20:06 PDT
> 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) 2006-05-14 10:53:43 PDT
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) 2006-05-14 10:54:42 PDT
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 [:bz] (still a bit busy) 2006-05-14 15:21:02 PDT
Created attachment 221989 [details] [diff] [review]
Updated to biesi's comments

> MangleKeywordIntoURI is just copied from elsewhere, right?

Yes, from the current keyword: protocol handler.
Comment 20 Boris Zbarsky [:bz] (still a bit busy) 2006-05-15 21:10:06 PDT
Fixed.  Rest in peace, little protocol handler.  Let your dreams be untroubled.
Comment 21 Hiro 2006-05-20 07:34:28 PDT
Created attachment 222720 [details] [diff] [review]
remove netwerk/protocol/keyword/ from allmakefiles.sh

netwerk/protocol/keyword/still remains in allmakefiles.sh.
Comment 22 Boris Zbarsky [:bz] (still a bit busy) 2006-05-30 11:03:17 PDT
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 2006-06-23 10:29:16 PDT
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 [:bz] (still a bit busy) 2006-06-23 10:32:37 PDT
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 2006-06-23 11:41:50 PDT
Yeah, just remove it! :)
Comment 26 Boris Zbarsky [:bz] (still a bit busy) 2006-06-23 21:44:55 PDT
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 2006-06-26 10:04:32 PDT
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 [:bz] (still a bit busy) 2006-06-27 18:08:32 PDT
Created attachment 227330 [details] [diff] [review]
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 [:bz] (still a bit busy) 2006-06-27 20:37:56 PDT
Fixed on 1.8.1 branch.

Note You need to log in before you can comment on or make changes to this bug.