Closed Bug 469302 Opened 12 years ago Closed 12 years ago

Page source code after form submit shows initial form page instead of new page

Categories

(Toolkit :: View Source, defect)

defect
Not set
major

Tracking

()

VERIFIED FIXED
mozilla1.9.1b3

People

(Reporter: flashcode, Assigned: stanley.appel)

References

()

Details

(Keywords: regression, testcase, verified1.9.1)

Attachments

(1 file, 2 obsolete files)

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.
Flags: blocking-firefox3.1?
Version: unspecified → 3.1 Branch
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.
Blocks: 17612
Status: UNCONFIRMED → NEW
Component: General → View Source
Ever confirmed: true
Flags: blocking-firefox3.1?
Keywords: regression, testcase
Product: Firefox → Toolkit
QA Contact: general → view.source
Version: 3.1 Branch → Trunk
Flags: blocking1.9.2?
Flags: blocking1.9.1?
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.
Duplicate of this bug: 469318
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.
Attached patch viewsource QueryInterface fix (obsolete) — Splinter Review
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.
Attachment #353062 - Flags: review?(cbartley)
Attachment #353062 - Flags: approval1.9.1?
(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.
Flags: blocking1.9.2?
Flags: blocking1.9.1?
Flags: blocking1.9.1+
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.
Attachment #353062 - Flags: review?(cbartley) → review+
Attachment #353062 - Flags: review+ → review?(gavin.sharp)
Attachment #353062 - Flags: review?(gavin.sharp) → review+
Whiteboard: checkin-needed
flagging in-testsuite?
Flags: 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
Attachment #353062 - Flags: superreview?(mrbkap)
Attachment #353062 - Flags: superreview?(mrbkap) → superreview+
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);
Attached patch viewsource QueryInterface fix v2 (obsolete) — Splinter Review
all in one line.
confirmed on 3.1b3 branch and current trunk
Attachment #353062 - Attachment is obsolete: true
Attachment #353422 - Flags: approval1.9.1?
Attachment #353062 - Flags: approval1.9.1?
Duplicate of this bug: 470005
Duplicate of this bug: 470357
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?
Attached patch Fix v.3Splinter Review
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.)
Attachment #353422 - Attachment is obsolete: true
Attachment #354790 - Flags: review?(gavin.sharp)
Attachment #353422 - Flags: approval1.9.1?
OS: Windows XP → All
Hardware: x86 → All
Whiteboard: checkin-needed
Target Milestone: --- → mozilla1.9.1b3
Attachment #354790 - Flags: review?(gavin.sharp) → review+
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 :)
Assignee: nobody → stanley.appel
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
(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!
Target Milestone: mozilla1.9.1b3 → mozilla1.9.2a1
Whiteboard: checkin-needed [needs to land on 1.9.1 branch]
https://hg.mozilla.org/releases/mozilla-1.9.1/rev/6b33245bbd2a
Whiteboard: checkin-needed [needs to land on 1.9.1 branch]
Target Milestone: mozilla1.9.2a1 → mozilla1.9.1b3
Blocks: 472632
Blocks: FF2SM
No longer blocks: FF2SM
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.
Status: RESOLVED → VERIFIED
Flags: in-litmus?
Depends on: 743254
You need to log in before you can comment on or make changes to this bug.