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

RESOLVED FIXED

Status

()

defect
RESOLVED FIXED
9 years ago
2 months ago

People

(Reporter: justin.lebar+bug, Assigned: justin.lebar+bug)

Tracking

unspecified
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking2.0 -)

Details

Attachments

(1 attachment, 1 obsolete attachment)

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
OS: Linux → All
Hardware: x86_64 → All
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.
blocking2.0: ? → -
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.
I say lets do it. I see no reason not to.
The good news is that this change is easy.

The bad news is that the test isn't.  Working on it.
Status: NEW → ASSIGNED
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?
Well, if the popup appears then the mochitest will hang right? Timing out is a valid way for tests to fail IMO.
That sounds good to me.  I'll try.
Posted patch Patch v1 (obsolete) — Splinter Review
Attachment #464881 - Flags: review?(jonas)
You should probably check that the reload succeeds. In case we make the popup non-blocking in the future.
(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?
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.
The dialog doesn't appear if the transaction takes place in an iframe.  Certainly the POST vs GET difference should still be there.
No dialog is even better! That way we'll get decent error messages rather than a timeout in case of a regression.
Posted patch Patch v2Splinter Review
All right; how's this look?
Attachment #464881 - Attachment is obsolete: true
Attachment #464972 - Flags: review?(jonas)
Attachment #464881 - Flags: review?(jonas)
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.
Attachment #464972 - Flags: review?(jonas) → review+
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.
Assignee: nobody → justin.lebar+bug
http://hg.mozilla.org/mozilla-central/rev/7ba410f45f42
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
Depends on: 589933
http://hg.mozilla.org/mozilla-central/rev/1a83b122acf2
Status: REOPENED → RESOLVED
Last Resolved: 9 years ago9 years ago
Resolution: --- → FIXED
No longer blocks: 580066
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?
You could disable pushState and replaceState by setting browser.history.allow{Push,Replace}State to false.
> 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.
(In reply to comment #25)
> > You could disable pushState and replaceState 
> 
> Uh.. we do NOT want extensions to do that, right?

Right!
(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?
Component: DOM → DOM: Core & HTML
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.