580069 - Calling replaceState doesn't clear the POST-ness of a history entry

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Justin Lebar (not reading bugmail)]

As discovered in bug 577720, if you:

  * POST to process_bug.cgi,
  * call replaceState to change the URL to show_bug.cgi, and
  * refresh the page

you'll receive the warning FF shows when you try and refresh a page which was POST'ed to.

Note that if you use pushState instead of replaceState, you won't get that warning.  (This isn't intentional.)

Perhaps the right thing to do here is to forget the POST state of a page after a replaceState call which changes the URI.

I've posted to the WHATWG about this [1], but haven't gotten much of a response yet.

[1] http://lists.whatwg.org/htdig.cgi/whatwg-whatwg.org/2010-July/027164.html

Comment 1

[Johnny Stenback (:jst)]

Not blocking on this, but if we get this sorted out before we ship Firefox 4 then we should take this fix, assuming it's safe enough etc.

Comment 2

[Justin Lebar (not reading bugmail)]

fwiw, Hixie seems to think this is worth trying out.  He says he'd be amenable to changing the spec to reflect this change if our experiment has positive results.

Comment 3

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I say lets do it. I see no reason not to.

Comment 4

[Justin Lebar (not reading bugmail)]

The good news is that this change is easy.

The bad news is that the test isn't.  Working on it.

Comment 5

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

You know about sjs files right?

https://developer.mozilla.org/En/Mochitest#How_do_I_change_the_HTTP_headers_or_status_sent_with_a_file_used_in_a_Mochitest.3f

Comment 6

[Justin Lebar (not reading bugmail)]

The trick isn't so much getting a POST'ed page into mochitest but checking that refreshing that page after a replaceState doesn't cause the "you're refreshing a POST'ed page" dialog to pop up.

Maybe it would be sufficient to test that the SHEntry's postData is null after
a replaceState?

Comment 7

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Well, if the popup appears then the mochitest will hang right? Timing out is a valid way for tests to fail IMO.

Comment 8

[Justin Lebar (not reading bugmail)]

That sounds good to me.  I'll try.

Comment 9

[Justin Lebar (not reading bugmail)]

Attachment: Patch v1

Comment 10

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

You should probably check that the reload succeeds. In case we make the popup non-blocking in the future.

Comment 11

[Justin Lebar (not reading bugmail)]

(In reply to comment #10)
> You should probably check that the reload succeeds. In case we make the popup
> non-blocking in the future.

Sure.  Is there a cleaner way to do this than adding a node to the page before refresh, doing the refresh, and then checking that the node is gone?

Comment 12

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

A better way is to have the loaded page call out to the other frame once it's done loading.

Basically do this:

Change from using a popup to using an <iframe>
Change opener.popupLoad(...) to parent.popupLoad(...)
Change file_bug580069_2.html from being a static file to being a sjs file which
  also outputs the http-method used in the request
In the |else if (num == "2")| branch, inspect the target document to see what
  request type was used. Make sure that it's "POST" the first time and "GET"
  the second time. Also check that the second time the query parameter is correct
  (using .location.search iirc).
  Only call SimpleTest.finish() the second time entering this branch.

Comment 13

[Justin Lebar (not reading bugmail)]

The dialog doesn't appear if the transaction takes place in an iframe.  Certainly the POST vs GET difference should still be there.

Comment 14

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

No dialog is even better! That way we'll get decent error messages rather than a timeout in case of a regression.

Comment 15

[Justin Lebar (not reading bugmail)]

Attachment: Patch v2

All right; how's this look?

Comment 16

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 464972 [details] [diff] [review]
Patch v2

>+function page2Load(method) {
...
>+  else {
>+    // Only run this function twice.
>+    return;
>+  }

I'd say add a |ok(false, "should only be called twice");| here.

Also, might want to drop the dump() calls, not sure if people care about the amount of spew being put out. Don't really care either way personally.

Comment 17

[Justin Lebar (not reading bugmail)]

If they're not too objectionable, I'd like to leave the dumps in.  In case the test hangs, they'll make things a bit easier to debug.

Comment 18

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Ok with me

Comment 19

[Justin Lebar (not reading bugmail)]

http://hg.mozilla.org/mozilla-central/rev/7ba410f45f42

Comment 20

[Justin Lebar (not reading bugmail)]

This bounced; it looks like the test I added was causing another test to fail somehow.

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1282171198.1282172815.8806.gz

Comment 21

[Justin Lebar (not reading bugmail)]

Upon more investigation, I don't think this patch is at fault for the test failure, so much as, in philor's words, it was "screwing up the delicate balance of an enormous pile of teeny straws."

See bug 552023.  I'm going to try to pull out the test fix from that patch.

Comment 22

[Justin Lebar (not reading bugmail)]

http://hg.mozilla.org/mozilla-central/rev/1a83b122acf2

Comment 23

[Frédéric Buclin]

In Firefox 3.6, I use the great UrlParams extension to track arguments and values passed to process_bug.cgi, for debugging purposes. But due to bug 577720 and this bug, UrlParams is no longer able to get arguments passed with the POST method, severely impacting my ability to debug Bugzilla.

How is such an extension (or me) supposed to work around this problem?

Comment 24

[Justin Lebar (not reading bugmail)]

You could disable pushState and replaceState by setting browser.history.allow{Push,Replace}State to false.

Comment 25

[Boris Zbarsky [:bzbarsky]]

> You could disable pushState and replaceState 

Uh.. we do NOT want extensions to do that, right?

Frédéric doing that should be ok, though.

Comment 26

[Justin Lebar (not reading bugmail)]

(In reply to comment #25)
> > You could disable pushState and replaceState 
> 
> Uh.. we do NOT want extensions to do that, right?

Right!

Comment 27

[[e-mail]]

(In reply to comment #23)
> In Firefox 3.6, I use the great UrlParams extension to track arguments and
> values passed to process_bug.cgi, for debugging purposes. But due to bug 577720
> and this bug, UrlParams is no longer able to get arguments passed with the POST
> method, severely impacting my ability to debug Bugzilla.
> 
> How is such an extension (or me) supposed to work around this problem?

Perhaps you could log the POST request at the HTTP level?