User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b2) Gecko/20081201 Firefox/3.1b2 Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b2) Gecko/20081201 Firefox/3.1b2 When I'm on a page which has a form with method "post", I click on "Submit", then I look at page source code (View => Page source), and I got HTML source for initial page (the one with the form). Reproducible: Always Steps to Reproduce: 1.go to http://seb.flashtux.org/bug_firefox_form_source/ 2.click on "submit" 3.look at page source Actual Results: Page source is the form (before submit/post). Expected Results: See page source of new page (after submit/post). With Firefox 3.0, I don't have this bug. I didn't test with Firefox 3.1 beta 1.

Confirmed on Windows XP, latest trunk build. Regression range: http://hg.mozilla.org/mozilla-central/pushloghtml?startdate=2008-11-04+17%3A00%3A00&enddate=2008-11-04+23%3A00%3A00 Bug 17612 seems to be the most likely culprit.

The error was indeed introduced by one of the patches for bug 17612, specifically Attachment 345657 [details] [diff]. I know the problem is somewhere inside viewSource.js, but I don't know where yet. I suspect the viewSource() function.

I suspect we run into https://bugzilla.mozilla.org/show_bug.cgi?id=17612#c33

If I reverse the following change in viewSource.js: @@ -170,6 +213,14 @@ // possible) rather than the network... // PageLoader.loadPage(arg, pageLoaderIface.DISPLAY_AS_SOURCE); + + // Record the page load in the session history so <back> will work. + var shEntry = Cc["@mozilla.org/browser/session-history-entry;1"].createInstance(Ci.nsISHEntry); + shEntry.setURI(makeURI(viewSrcUrl, null, null)); + shEntry.setTitle(viewSrcUrl); + shEntry.loadType = Ci.nsIDocShellLoadInfo.loadHistory; + getBrowser().webNavigation.sessionHistory.addEntry(shEntry, true); + the view source works again like it should.

Found a fix. There was too much cleanup based on the comments on https://bugzilla.mozilla.org/show_bug.cgi?id=17612#c31 IMHO there is a QueryInterface(Ci.nsISHistoryInternal) missing in the code. Tested this on my on testcase and the one given on this bug.

(In reply to comment #6) > Created an attachment (id=353062) [details] > viewsource QueryInterface fix > > Found a fix. > There was too much cleanup based on the comments on > https://bugzilla.mozilla.org/show_bug.cgi?id=17612#c31 > IMHO there is a QueryInterface(Ci.nsISHistoryInternal) missing in the code. > Tested this on my on testcase and the one given on this bug. Stappel: I'll test the patch out later today. If it's really this simple, then there should be no problem getting it approved. Thanks for looking into this.

I took a deeper look since it looked like this code (before Stappel's patch) worked in some cases and not in others, which seemed kind of surprising. Turns out that it just never worked period. Basically the code is designed to load from cache if at all possible. However, if it fails to load from cache because of an exception the exception is caught by an empty exception handler, which lets control flow fall out and into the general reload-the-file-from-the-internet case. No attempt is made to discriminate between "runtime" exceptions and logic exceptions, causing the code to (in this case) bury an important error. In most cases, it's not really possible to tell through the UI if a file came from cache or was reloaded. So although this functionality was broken from the beginning, it wasn't until someone tried a form that posted back to its source URL for the bug to appear obvious. Just to be clear, the design with the empty catch blocks existed in the code before I ever modified it to support linkification. I decided to preserve the basic way the code worked because I wanted to minimize the changes I needed to make. However, I knew the empty catch-block design was problematical, because I had alerts in some of them (there are more in that file!) while I was working on the code. Besides just fixing the source problem here, there are a couple of other things we should do or at least seriously consider: 1. Add a specific test case to the test plan for forms that post back to the same address they originally loaded from (this bug). 2. Add a new bug to clean up the exception handling in viewSource.js post FF 3.1. 3. Maybe consider modifying the UI in some way so that it's possible to tell when loads come from cache.

flagging in-testsuite?

(In reply to comment #8) > 1. Add a specific test case to the test plan for forms that post back to the > same address they originally loaded from (this bug). https://wiki.mozilla.org/QA/Firefox3.1/ViewSource_Testplan#Is_the_page_source_being_retrieved_from_the_cache.3F > 2. Add a new bug to clean up the exception handling in viewSource.js post FF > 3.1. Bug 469762 - Clean up exception handlers in toolkit/components/viewsource/content/viewSource.js

Comment on attachment 353062 [details] [diff] [review] viewsource QueryInterface fix It seems like you could combine the two lines, so: getBrowser().webNavigation.sessionHistory.QueryInterface(Ci.nsISHistoryInternal).addEntry(shEntry, true);

all in one line. confirmed on 3.1b3 branch and current trunk

See comments in bug 470357 about other ways this code should probably be bullet-proofed. I'd also like us to add some tests so this doesn't happen again.

We need this to bake on trunk before we can take it on branch, at least for one cycle of the tinder- and talos boxen. Can we get that checkin taken care of, please?

I hadn't been checking it in because checking something in below a comment from bz saying that something else needs to be done is the sort of thing I like to avoid, but when I was going to try to anyway, I see that it hasn't been checked in yet because the patch is malformed and won't apply. So, while I was having to redo it, I wrapped it and moved the line bz wanted moved, and changed the comment because I haven't got a clue what "loaded from the page cookie" might mean, so it's now pretty much a completely different patch other than the idea, so anal though it is, r? again. (And it shouldn't need approval1.9.1 unless it takes a really long time to land, since it's blocking1.9.1.)

http://hg.mozilla.org/mozilla-central/rev/eba3f74d7734 Now, don't fail me by not fulfilling my claim that a wiki page talking about a test plan is close to being nearly as good as the test the patch should have had to be able to land :)

(In reply to comment #16) > We need this to bake on trunk before we can take it on branch.... I'll move up from ShireTokoto to Minefield as soon as I'm done with this post, and help with the 'baking'. (I'll be "testing" in normal usage and the process within my duplicate bug 470005, which does play around with various Cookie tricks.) My thanks to everyone!

(In reply to comment #19) Sorry to be 2 days late with completing this verification. But, Minefield shows correct page source WITHOUT FLAW through all 32 "steps" of the procedure in bug 470005. nice work!

verified fixed on the 1.9.1 branch using Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b3pre) Gecko/20090108 Shiretoko/3.1b3pre and Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b3pre) Gecko/20090108 Shiretoko/3.1b3pre verified fixed on the trunk using Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090108 Minefield/3.2a1pre. Probably wouldn't hurt to have a Litmus test for this so nominating for consideration.