48270 - Links don't load, Assertions clicking on links

Status: VERIFIED DUPLICATE of bug 48200

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

Description

[Mike Pinkerton (not reading bugmail)]

Load CNN, click on the "US" news link on the left side of the page. You get the 
following assertions:

###!!! ASSERTION: null uri arg: 'aURI', file nsDocShell.cpp, line 3129
###!!! ASSERTION: NS_ENSURE_TRUE(NS_SUCCEEDED(ScrollIfAnchor(aURI, &wasAnchor))) 
failed: '(!((ScrollIfAnchor(aURI, &wasAnchor)) & 0x80000000))', file 
nsDocShell.cpp, line 2723

The link does not load, nothing happens.

Comment 1

[Mike Pinkerton (not reading bugmail)]

i now can't load most of the pages on cnn's front page. this has gotten really 
bad. anyone? anyone?

Comment 2

[leger]

Putting on [dogfood+][nsbeta3+] radar

Comment 3

[Judson Valeski]

chris, I was testing something else the other day and hit this same assertion 
chain. I was testing how we handled bogus uri schemes by putting an href 
pointing to "foobar://blah". The problem is that layout isn't creating a uri for 
some reason (in my test case it's obvious why) when it fires the OnClick handler 
so we get to the docshell and it doesn't have a uri to load.

Comment 4

[vidur (gone)]

http://lxr.mozilla.org/seamonkey/source/layout/base/src/nsGenericElement.cpp#236
7 is where we try to create a nsIURI instance based on a link click.

Comment 5

[Judson Valeski]

related to 37416?

Comment 6

[Adam Lock]

I'm seeing the correct behaviour for this page today. It's not clear from the 
generic element code how it could fail on this one site without failing on all 
other links too.

Comment 7

[Mike Pinkerton (not reading bugmail)]

this still happens for me on mac, fresh brand new profile, build from 8/11/00.

Comment 8

[Mike Pinkerton (not reading bugmail)]

i also say this on espn.com today (8/14).

Comment 9

[Mike Pinkerton (not reading bugmail)]

was able to dupe on win32 on 8/14

marking all platforms.

Comment 10

[Chris Waterson]

WORKSFORME on Linux, 2000-08-14. Tried several links on ESPN and CNN and they
all worked fine.

Comment 11

[Mike Pinkerton (not reading bugmail)]

chris, i can dupe this with both mac and win32. come by my cube if you want.

Comment 12

[Chris Waterson]

pink, this WORKSFORME on all platforms. Seems to be working for other 
people, as well. Could you verify that your component registry is ok, and that 
you don't have any mods in your tree? I'll come by and look at it on your 
machine when I get in.

Comment 13

[Mike Pinkerton (not reading bugmail)]

sorry everyone, my bad. the way to see this 100%  is to just type

cnn

in the url bar, _not_ www.cnn.com. this probably related to 48636 also which 
happens only when you don't type the full name.

Comment 14

[Chris Waterson]

The document's base URL is "keyword:cnn".

When you just type "cnn" into the URL bar, we go off to the internet keyword
server to determine the URL that we should redirect to. Unfortunately, we don't
seem to set the new document's URL to the redirect URL: we used the "keyword:"
URL instead. This will break image loading and anything else that relies on
being document relative.

Kicking back over to the docshell component...

Comment 15

[Adam Lock]

Ok, I can reproduce too now. I was typing www.cnn.com before.

Comment 16

[Adam Lock]

This appears to be the sequence for keywords:

1. Someone types in the keyword, e.g. "cnn"
2. nsDocshell decides to fix it up as a keyword, -> "keyword:cnn"
3. The keyword protocol handler is invoked to open a channel. It transforms
   the keyword into a URL and opens an HTTP channel on it,
   e.g. "http://keyword.netscape.com/keyword/cnn"
4. The keyword server looks up the keyword and sends a
   "HTTP/1.1 302 Moved Temporarily" response
5. The nsHTTPChannel detects the redirect to "http://www.cnn.com", and
   creates a new channel which it fixes up with the URI, but LEAVES THE
   ORIGINAL URI THE SAME.
6. Document is loaded as normal, with the base URL set to the original URI
   on the channel, i.e. "keyword:cnn"
7. Link over events screw up because the document base URL is duff

It seems to me that the way to fix this is to change the redirect code in the 
http channel so that the original uri and the new uri are both set to the same 
value. Is this reasonable?

Proposed patch follows
   

Comment 17

[Adam Lock]

Attachment: Redirect sets uri and original uri to be the same thing on new channel

Comment 18

[Adam Lock]

CC'ing Jud as for review/approval purposes

Comment 19

[Judson Valeski]

cc'ing gagan as he should review the http patch

Comment 20

[Gagan]

ahhh... this is really a dup of 48200. Wish I had seen this earliar to avoid 
duplication of effort. Otherwise this fix would additionally help to the fix for 
bug 48200. 

*** This bug has been marked as a duplicate of 48200 ***

Comment 21

[Gagan]

In retrospect I am a little uneasy about setting the originalURI to be the same 
as the current URI since this changes the semantics of how the other people 
(security, etc.) have been using this. 

Comment 22

[Adam Lock]

The problem in this instance is that we have no other choice but to set the URIs 
to be the same. The loaded document appends relative URIs to the original URI 
for links and so on. Neither the original URI "keyword:cnn", nor the resolved 
URI before the redirect "http://keyword.netscape.com/keyword/cnn" make any 
sense. Relative URIs should be appended to the the redirected URI, 
"http://www.cnn.com"
 
What are the security issues?

Comment 23

[Judson Valeski]

well, I think "we have no other choice" is pretty strong here. I had envisioned 
the app (docshell) figuring out that a re-direct was happening (or had happened) 
and that HTTP would push the new channel (and subsequently new URI) up somehow 
(that "somehow" part isn't defined :-/). Gagan?

Comment 24

[Adam Lock]

This is pretty much what does happen, all except the bit where the docshell gets 
told when its happened. The HTTP channel detects the redirect, creates a new 
channel to the new URL, sets it up with all the subscriptions and settings and 
then aborts to leave the new channel to push out the data.

There is a call out to OnRedirect on any registered nsIHTTPEventSink interfaces 
at the end of all this but no one implements this interface.

Comment 25

[Gagan]

the nsIHTTPEventSink implementation is a separate issue. But the bigger problems
lies with chrome and resource URLs. Current implementation of these require us
to know when they have been redirected to an HTTP or a file URL and that would
not be possible if we reset the OriginalURI. 

I agree that OriginalURI semantics has been badly abused but thats not the way
to fix it. The fix that I have for the nsDocument takes care of this in the way
it should be (at least for now-- till someone comes up with a better design
altogether)

Comment 26

[Gagan]

Oh... And I forgot to add, the security issues emerge from the permissions and
such. Security may have a different permission for the originalURI than the URI
and hence the possible breakage there.

Comment 27

[Diego]

Verified dupe of fixed bug 48200: "Incorrect base URL used on redirected pages"