Beginning on October 25th, 2016, Persona will no longer be an option for authentication on BMO. For more details see Persona Deprecated.
Last Comment Bug 415078 - nsWebShell.cpp attempts to fixup immutable URIs
: nsWebShell.cpp attempts to fixup immutable URIs
Product: Core
Classification: Components
Component: Document Navigation (show other bugs)
: Trunk
: All All
: -- minor (vote)
: mozilla1.9beta4
Assigned To: Graeme McCutcheon [:graememcc]
Depends on:
  Show dependency treegraph
Reported: 2008-01-31 07:26 PST by Graeme McCutcheon [:graememcc]
Modified: 2008-02-07 01:08 PST (History)
3 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

Patch v1 for review (1.53 KB, patch)
2008-01-31 07:45 PST, Graeme McCutcheon [:graememcc]
bzbarsky: review-
Details | Diff | Splinter Review
Patch v2 for review (1.11 KB, patch)
2008-01-31 11:39 PST, Graeme McCutcheon [:graememcc]
bzbarsky: review+
bzbarsky: superreview+
mtschrep: approval1.9+
Details | Diff | Splinter Review

Description Graeme McCutcheon [:graememcc] 2008-01-31 07:26:43 PST
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] 2008-01-31 07:45:49 PST
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)
Comment 2 Boris Zbarsky [:bz] (still a bit busy) 2008-01-31 08:20:12 PST
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] 2008-01-31 11:39:28 PST
Created attachment 300699 [details] [diff] [review]
Patch v2 for 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?
Comment 4 Boris Zbarsky [:bz] (still a bit busy) 2008-01-31 20:59:34 PST
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 [:bz] (still a bit busy) 2008-01-31 21:05:23 PST
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 2008-02-06 23:52:07 PST
Comment on attachment 300699 [details] [diff] [review]
Patch v2 for review

Bz thanks for the explination
Comment 7 Reed Loden [:reed] (use needinfo?) 2008-02-07 01:08:35 PST
Checking in docshell/base/nsWebShell.cpp;
/cvsroot/mozilla/docshell/base/nsWebShell.cpp,v  <--  nsWebShell.cpp
new revision: 1.701; previous revision: 1.700

Note You need to log in before you can comment on or make changes to this bug.