User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b8pre) Gecko/20101125 Firefox/4.0b8pre Build Identifier: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b8pre) Gecko/20101125 Firefox/4.0b8pre When you pick a specialty pizza from Domino's menu it is supposed to forward you to http://express.dominos.com/order/olo.jsp#storeMenu-entrees-BuildSpecialtyPizza with the toppings pre-selected so you can choose what you want (just verified in a non-Mozilla browser). The latest nightlies of Minefield instead forward you to http://express.dominos.com/order/olo.jsp#storeMenu-entrees-BuildPizza which is the generic pizza builder, without the toppings pre-selected and without the specialty pizza being noted as such on your order summary. Reproducible: Always Steps to Reproduce: Try to order a specialty pizza from Domino's. I am running Flash 10.1.102.64.

When did this stop working correctly? I'd probably be able to answer the question myself, by the way, if I knew how to reproduce this, but the steps to reproduce are not really usable for someone who has never ordered a specialty pizza from Domino's. What does one need to do after loading the URL in the URL field to reproduce the bug?

Forgot to give full instructions. This stopped working about a week ago. It might be a site redesign problem rather than a browser problem, but it does work in non Mozilla browsers. 1. Go to www.dominos.com 2. Click order 3. Enter an address. anything, even 123 Fake street, zip 90210 would work 4. Click continue 5. choose one and click order online now. if the store is closed that's ok, you can click place future order instead. 6. Click entrees and then Specialty Pizza 7. Select any of them and click add to order 8. In IE 8, it will bring up a pizza builder with toppings pre-selected. In Firefox as of about a week ago it no longer works. It brings up a different page with the toppings not selected. I'm not a programmer but it looks like the right information isn't being sent back to the server, which is why I initially filed this under cookies.

Thanks for the steps to reproduce! I can definitely reproduce with those on trunk, but not in 3.6; looking for the regression range now.

Regression range seems to be http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=4be7f43c1de3&tochange=97745a2b2de9 which implies that there was in fact a recent web site redesign, but the new design does work in 3.6...

I wonder whether this is a regression from bug 515190.

Yep, making the hashchange call async, as it was in 3.6, fixes the site. jlebar, is this something the spec absolutely requires? If so, can the spec be changed? Or do we need to get in touch with Domino's about this? Da' Nard, thanks for the bug report!

And in fact, looks like the spec change to fire hashchange sync has already been reverted; see bug 515190 comment 17.

Firing hashchange async sounds fine to me. It looks like the spec has been relatively stable in this regard recently.

Great, but don't nuke my changes! Especially the blocker nomination.

(In reply to comment #9) > Great, but don't nuke my changes! Especially the blocker nomination. I didn't get a midair warning, and I'm pretty sure I refreshed the page right before I typed the comment. Strange.

If you refreshed but didn't force-refresh, then you'd get no midair warning _and_ clobber the changes.

I generated this patch by backing out changeset c922c6402f76, the checkin from bug 515190.

> NS_ASSERTION(nsContentUtils::IsSafeToRunScript(), > "Must be safe to run script here."); Should this go in DispatchAsyncHashchange() or in FireHashchange()?

I wish these tests I wrote weren't so fragile. Oh well. I added dumps to the test since it apparently went orange twice a few months ago (bug 584381).

Comment on attachment 493639 [details] [diff] [review] Patch v2: Adding test fix. This changes nsPIDOMWindow, but doesn't update IID. And what are the changes to nsDocshell.h?

(In reply to comment #17) > This changes nsPIDOMWindow, but doesn't update IID. We might have missed that in the original patch! Fixed now. Are we still allowed to change this interface? I can't keep the rules straight. > And what are the changes to nsDocshell.h? A very poor merge on my part that was surprisingly not caught by the compiler. > NS_ASSERTION(nsContentUtils::IsSafeToRunScript(), > "Must be safe to run script here."); This assertion isn't needed in the async case, right?

(In reply to comment #18) > Are we still allowed to change this interface? I can't keep the rules > straight. I think changes to interfaces aren't allowed. > This assertion isn't needed in the async case, right? Yeah, not needed.

Comment on attachment 493645 [details] [diff] [review] Patch v3 >diff --git a/docshell/base/nsDocShell.cpp b/docshell/base/nsDocShell.cpp >--- a/docshell/base/nsDocShell.cpp >+++ b/docshell/base/nsDocShell.cpp >@@ -8332,17 +8332,17 @@ nsDocShell::InternalLoad(nsIURI * aURI, > SetDocPendingStateObj(mOSHE); > > // Dispatch the popstate and hashchange events, as appropriate > nsCOMPtr<nsPIDOMWindow> window = do_QueryInterface(mScriptGlobal); > if (window) { > window->DispatchSyncPopState(); > > if (doHashchange) >- window->DispatchSyncHashchange(); >+ window->DispatchAsyncHashchange(); It would be a bit ugly, but could you take the inner window here and then dispatch a runnable which calls DispatchSyncHashchange. That way you wouldn't need to change the interface.

I think this particular API change is ok without changing the IID. The result is binary-compatible, the name change is irrelevant, and the semantics change is no different than if we just changed the impl without changing the name.... It _could_ confuse an someone who called it expecting an actual sync hashchange, but I'm willing to not worry about that.

But yes, putting the runnable in the docshell code is what I did to test in comment 6. It's not even all that ugly; it's a one-line change.

Yeah, make this change without changing the IID and we should be fine.

Comment on attachment 493680 [details] [diff] [review] Patch v4 - No IID change Could you remove the "dump('111');" kind of debug code. And fix the indentation after popup.QueryInterface ...

From comment 16: > I added dumps to the test since it apparently went orange twice a few months > ago (bug 584381). Do you still think we should get rid of them? I don't really care, but it would have helped debugging a similar randomorange. I can make it dump fewer characters, if that's better. :)

Ah, well, rename the dumps to something else. '111', '222' are just too generic. And dump also a newline.

Sadly the checkin comment is wrong. It should be "dispatch the hashchange event *asynchronously*".

(In reply to comment #30) > Sadly the checkin comment is wrong. It should be "dispatch the hashchange > event *asynchronously*". You could always backout and reland with the correct comment :)