Closed Bug 415078 Opened 14 years ago Closed 14 years ago
Web Shell .cpp attempts to fixup immutable URIs
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.
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
Status: NEW → ASSIGNED
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. 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?
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.
Comment on attachment 300699 [details] [diff] [review] Patch v2 for review Bz thanks for the explination
Attachment #300699 - Flags: approval1.9? → approval1.9+
Checking in docshell/base/nsWebShell.cpp; /cvsroot/mozilla/docshell/base/nsWebShell.cpp,v <-- nsWebShell.cpp new revision: 1.701; previous revision: 1.700 done
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
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.