nsWebShell.cpp attempts to fixup immutable URIs

RESOLVED FIXED in mozilla1.9beta4

Status

()

Core
Document Navigation
--
minor
RESOLVED FIXED
10 years ago
10 years ago

People

(Reporter: graememcc, Assigned: graememcc)

Tracking

Trunk
mozilla1.9beta4
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

10 years ago
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.
(Assignee)

Comment 1

10 years ago
Created attachment 300644 [details] [diff] [review]
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)
Assignee: nobody → graememcc_firefox
Status: NEW → ASSIGNED
Attachment #300644 - Flags: review?(bzbarsky)

Comment 2

10 years ago
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-
(Assignee)

Comment 3

10 years ago
Created attachment 300699 [details] [diff] [review]
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?
Attachment #300644 - Attachment is obsolete: true
Attachment #300699 - Flags: review?(bzbarsky)

Comment 4

10 years ago
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

10 years ago
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?
(Assignee)

Updated

10 years ago

Comment 6

10 years ago
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
done
Status: ASSIGNED → RESOLVED
Last Resolved: 10 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.