Closed Bug 415078 Opened 15 years ago Closed 15 years ago

nsWebShell.cpp attempts to fixup immutable URIs


(Core :: DOM: Navigation, defect)

Not set





(Reporter: graememcc, Assigned: graememcc)





(1 file, 1 obsolete file)

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.
Attached patch Patch v1 for review (obsolete) — Splinter 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)
Assignee: nobody → graememcc_firefox
Attachment #300644 - Flags: review?(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.
Attachment #300644 - Flags: review?(bzbarsky) → review-
On further investigation, the SetSpec seems to have appeared arbitrarily in 1.506.

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?
Attachment #300644 - Attachment is obsolete: true
Attachment #300699 - Flags: review?(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 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.
Attachment #300699 - Flags: superreview+
Attachment #300699 - Flags: review?(bzbarsky)
Attachment #300699 - Flags: review+
Attachment #300699 - Flags: approval1.9?
Comment on attachment 300699 [details] [diff] [review]
Patch v2 for review

Bz thanks for the explination
Attachment #300699 - Flags: approval1.9? → approval1.9+
Keywords: checkin-needed
Checking in docshell/base/nsWebShell.cpp;
/cvsroot/mozilla/docshell/base/nsWebShell.cpp,v  <--  nsWebShell.cpp
new revision: 1.701; previous revision: 1.700
Closed: 15 years ago
Keywords: checkin-needed
OS: Windows XP → All
Hardware: PC → All
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9beta4
You need to log in before you can comment on or make changes to this bug.