485196 - Web page generated by POST is retried as GET when Save Frame As used, and the page is no longer in the cache

Status: RESOLVED FIXED

(Firefox :: File Handling, defect)

Description

[:Paolo Amadini]

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

Comment 1

[:Paolo Amadini]

Attachment: Explanation of the concept (modification of test for bug 471962)

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.

Comment 2

[Boris Zbarsky [:bzbarsky]]

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.

Comment 3

[Mike Beltzner [:beltzner, not reading bugmail]]

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

Comment 4

[Boris Zbarsky [:bzbarsky]]

> 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.

Comment 5

[Mike Beltzner [:beltzner, not reading bugmail]]

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

Comment 6

[:Paolo Amadini]

Attachment: The fix

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!

Comment 7

[Boris Zbarsky [:bzbarsky]]

> 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...

Comment 8

[Boris Zbarsky [:bzbarsky]]

Yes, we do.

Comment 9

[Boris Zbarsky [:bzbarsky]]

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

Comment 10

[:Paolo Amadini]

(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)

Comment 11

[Boris Zbarsky [:bzbarsky]]

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

Comment 12

[:Paolo Amadini]

Attachment: Patch and test case

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 13

[Johnny Stenback (:jst)]

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.

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

I can review this later this week.

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 434841 [details] [diff] [review]
Patch and test case

Actually some toolkit reviewer should look at this too.

Comment 16

[Dão Gottwald [:dao]]

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.

Comment 17

[:Paolo Amadini]

(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 18

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 434841 [details] [diff] [review]
Patch and test case

The wrapping in comment 17 looks good to me.

Comment 19

[:Gavin Sharp [email: [e-mail]]]

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().

Comment 20

[:Paolo Amadini]

(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. :-)

Comment 21

[Johnny Stenback (:jst)]

This is ready to land now, right?

Comment 22

[:Gavin Sharp [email: [e-mail]]]

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

that is fine!

Comment 23

[:Paolo Amadini]

Attachment: Patch and test case

(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.

Comment 24

[Jonathan Kew [:jfkthame]]

http://hg.mozilla.org/mozilla-central/rev/99271495668e

Comment 25

[Shawn Wilsher :sdwilsh]

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