415078 - nsWebShell.cpp attempts to fixup immutable URIs

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect)

Description

[Graeme McCutcheon [:graememcc]]

When nsWebShell::EnterPageLoad is called with a page load failure, it will attempt to fixup the URI where possible.

In some cases, eg view-source: urls, the URI object will be immutable, and when an attempt is made to set the URI to the fixed up spec you hit:

WARNING: NS_ENSURE_TRUE(mMutable) failed: 
file c:/mozilla/netwerk/base/src/nsSimpleURI.cpp, line 149

which causes EnterPageLoad to exit without displaying proper information to the user.

nsWebShell::EnterPageLoad should only attempt to fixup URIs if it is actually going to be possible to modify the URI object.

Comment 1

[Graeme McCutcheon [:graememcc]]

Attachment: Patch v1 for review

Simple patch for review, which simply stops attempting to fixup when the URI can't be modified.

Incidentally, I think this gives the rest of bug 318900 for free (I believe some of it was already fixed in bug 393002)

Comment 2

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 300644 [details] [diff] [review]
Patch v1 for review

The right fix is to fix up correctly (clone the URI, and fix up the clone), not to skip fixup of URIs altogether.  Especially because we might soon make HTTP URIs immutable.

Then again, the fixup itself isn't the problem.  It's the SetSpec afterwards, right before we do the new URI load that's the issue, no?  I don't see why that call is even needed, to be honest.

Comment 3

[Graeme McCutcheon [:graememcc]]

Attachment: Patch v2 for review

On further investigation, the SetSpec seems to have appeared arbitrarily in 1.506.

http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/docshell/base/nsWebShell.cpp&rev=1.505#1071
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/docshell/base/nsWebShell.cpp&rev=1.5063#1080

Indeed, there does not appear to be any reason for it.

Revised patch simply removes this assignment.

This does indeed give the rest of 318900. (However, it makes bug 66183 apply instead!)

Boris: could you also confirm if bug 60768 is a dupe of bug 109309?

Comment 4

[Boris Zbarsky [:bzbarsky]]

Actually, the code was there in both rev 1.505 and rev 1.506.  It just got moved.  At the time, the code made sense, since it did a SetHost() and then a GetSpec(), so basically used the existing URI to parse the hostname.

Then when bug 109309 landed the URI fixup setup changed, and since then this doesn't look like it should be needed.

I'm not really sure why this patch would fix bug 318900.  That seems pretty odd.  It's definitely the right thing to do, though, so we should do it.

> Boris: could you also confirm if bug 60768 is a dupe of bug 109309?

Indeed.  Marked accordingly.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 300699 [details] [diff] [review]
Patch v2 for review

Let's do this, yeah.

Approvers: this removes some code that basically changes the URI of the channel that just failed to load for no reason that we can see.  I believe the risk of this change is very low, especially if it has been verified that the affected error pages still work as they should.

Comment 6

[Mike Schroepfer]

Comment on attachment 300699 [details] [diff] [review]
Patch v2 for review

Bz thanks for the explination

Comment 7

[Reed Loden [:reed]]

Checking in docshell/base/nsWebShell.cpp;
/cvsroot/mozilla/docshell/base/nsWebShell.cpp,v  <--  nsWebShell.cpp
new revision: 1.701; previous revision: 1.700
done