User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.7) Gecko/2009021910 Firefox/3.0.7 (.NET CLR 3.5.30729) Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20090321 Minefield/3.6a1pre This bug is complementary to bug 300032 (= Text Web page generated by POST is retried as GET when Save As or Send Page used). Bug 300032 is about the fact that, when saving a page, the POST data from the history buffer is never resent for some document types. This bug is about retrieving the POST data from the correct entry in the history buffer (nsISHEntry), if available, for the current document; at present, this doesn't work correctly for subframes. Both bugs occur only when the page being saved is not in the cache. It is important to note that the POST data, if found, is now resent silently; bug 479296 should be fixed before attempting to fix this one. Reproducible: Always

This patch is a modification of an existing test case, just to explain the concept. It is not intended as a definitive test case for this bug.

The issue is that we don't get the postdata correctly for subframes, because we look at the docshell's session history, which is null for subframes. Over to File Handling, though ideally this would be in Toolkit:FileHandling.

bz: what would be required to fix this? the dependencies look kinda frightening.

> bz: what would be required to fix this? Getting the right session history entry or POST data stream. This probably involves either adding some API on docshell or some such or manually walking the session history looking for the right thing. The dependencies aren't really required to fix this bug; bug 486921 is one possible way to fix this but not the only one and bug 479296 would just bite in more cases if we fixed this, but already bites in plenty.

Paolo: would you be able to fix this? Won't block the release, but we'd love to take a fix for this.

This patch is likely to fix the bug. There is not a stringent need to modify the backend code, since we can get to the session history entry through the nsIWebPageDescriptor interface. Note that nsIWebPageDescriptor is not a good name for the interface, since it provides functions that give access to or use the "descriptor", but it is NOT the descriptor itself. The descriptor is an opaque nsISupports and currently corresponds to a session history entry (either mLSHE or mOSHE in the docshell). Notes: * The patch compiles and runs but I didn't really test it in the latest trunk. * SeaMonkey will need a separate patch, until bug 484616 is fixed. * Any test case written for this has the potential to break on SeaMonkey, until bug 484616 is fixed. * I'm striving to align contentAreaUtils.js between Firefox and SeaMonkey, in order to fix bug 484616, and every change makes it harder to do it (not more complex, just more cumbersome for me). * Fixing bug 486921 would allow us to get rid of the details of POST data handling in the front-end entirely, as well as fixing other bugs. Notwithstanding the above, feel free to test the patch and get it in the tree in order to fix this bug!

> we can get to the session history entry through the nsIWebPageDescriptor > interface. Oh, yeah. So much for adamlocks' opacity stuff... ;) Do we copy postdata when cloning SHEntries? I assume we do...

Yes, we do.

The patch does look good, if we're ok with the nasty bug 479296 issue...

(In reply to comment #7) > Do we copy postdata when cloning SHEntries? I assume we do... > Yes, we do. http://mxr.mozilla.org/mozilla-central/source/docshell/shistory/src/nsSHEntry.cpp#123 I don't understand if, being mPostData a pointer to an nsIInputStream, the two objects end up referencing the same stream? (it should not be a problem anyway as long as the stream is not accessed concurrently)

It does reference the same stream, and that's ok.

Now that bug 484616 is fixed, this patch and browser-chrome test case should fix the issue in Firefox and work fine in SeaMonkey too :-) Bug 479296 remains (it has been marked with the "uiwanted" keyword, but this word seems not to have any magic effect that causes the bug to progress...)

Comment on attachment 434841 [details] [diff] [review] Patch and test case Smaug, could you possibly review this? Trying to unload bz a bit here. If you're not comfortable reviewing this let me know.

I can review this later this week.

Comment on attachment 434841 [details] [diff] [review] Patch and test case Actually some toolkit reviewer should look at this too.

Comment on attachment 434841 [details] [diff] [review] Patch and test case >- var sessionHistory = aDocument.defaultView >- .QueryInterface(Components.interfaces.nsIInterfaceRequestor) >- .getInterface(Components.interfaces.nsIWebNavigation) >- .sessionHistory; >- return sessionHistory.getEntryAtIndex(sessionHistory.index, false) >- .QueryInterface(Components.interfaces.nsISHEntry) >- .postData; >+ // Find the session history entry corresponding to the given document. In >+ // the current implementation, nsIWebPageDescriptor.currentDescriptor always >+ // returns a session history entry. >+ var sessionHistoryEntry = aDocument.defaultView. >+ QueryInterface(Components.interfaces.nsIInterfaceRequestor). >+ getInterface(Components.interfaces.nsIWebNavigation). >+ QueryInterface(Components.interfaces.nsIWebPageDescriptor). >+ currentDescriptor. >+ QueryInterface(Components.interfaces.nsISHEntry); >+ return sessionHistoryEntry.postData; The indentation looks slightly strange, I think the prevailing style as used in attachment 399525 [details] [diff] [review] would be preferable.

(In reply to comment #16) > The indentation looks slightly strange, I think the prevailing style as used in > attachment 399525 [details] [diff] [review] would be preferable. Yes, in fact the style guide has also been updated meanwhile to recommend writing "." operators after the line break. I'd like to stay within 80 characters however, what do you think of the following solution? + var sessionHistoryEntry = + aDocument.defaultView + .QueryInterface(Components.interfaces.nsIInterfaceRequestor) + .getInterface(Components.interfaces.nsIWebNavigation) + .QueryInterface(Components.interfaces.nsIWebPageDescriptor) + .currentDescriptor + .QueryInterface(Components.interfaces.nsISHEntry);

Comment on attachment 434841 [details] [diff] [review] Patch and test case The wrapping in comment 17 looks good to me.

Comment on attachment 434841 [details] [diff] [review] Patch and test case >diff --git a/toolkit/content/tests/browser/browser_bug471962.js b/toolkit/content/tests/browser/browser_save_resend_postdata.js >+ // Check if outer POST data is found (bug 471962). >+ ok(fileContents.indexOf("inputfield=outer") === -1, > "The saved inner frame does not contain outer POST data"); >+ // Check if inner POST data is found (bug 485196). >+ ok(fileContents.indexOf("inputfield=inner") > -1, >+ "The saved inner frame was generated using the correct POST These should probably be using is() and isnot().

(In reply to comment #19) > (From update of attachment 434841 [details] [diff] [review]) > >diff --git a/toolkit/content/tests/browser/browser_bug471962.js b/toolkit/content/tests/browser/browser_save_resend_postdata.js > > >+ // Check if outer POST data is found (bug 471962). > >+ ok(fileContents.indexOf("inputfield=outer") === -1, > > "The saved inner frame does not contain outer POST data"); > > >+ // Check if inner POST data is found (bug 485196). > >+ ok(fileContents.indexOf("inputfield=inner") > -1, > >+ "The saved inner frame was generated using the correct POST > > These should probably be using is() and isnot(). Hmm, I think that these checks should be seen more like boolean expressions, we don't care what is the "actualValue" returned by indexOf if it's not -1. I'd keep these "ok"s as they are if you're "ok" with this. :-)

This is ready to land now, right?

(In reply to comment #20) > I'd keep these "ok"s as they are if you're "ok" with this. :-) that is fine!

(In reply to comment #21) > This is ready to land now, right? I was only waiting to make the indentation changes and check that last thing regarding the "ok"s. The patch is ready now.

Not a regression, so I don't think this blocks. Luckily, it's already fixed!