615061 - Domino's Pizza website does not work correctly due to hashchange now being sync

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect)

Description

[Da' Nard]

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.

Comment 1

[Boris Zbarsky [:bzbarsky]]

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?

Comment 2

[Da' Nard]

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.

Comment 3

[Boris Zbarsky [:bzbarsky]]

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.

Comment 4

[Boris Zbarsky [:bzbarsky]]

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

Comment 5

[Boris Zbarsky [:bzbarsky]]

I wonder whether this is a regression from bug 515190.

Comment 6

[Boris Zbarsky [:bzbarsky]]

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!

Comment 7

[Boris Zbarsky [:bzbarsky]]

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

Comment 8

[Justin Lebar (not reading bugmail)]

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

Comment 9

[Boris Zbarsky [:bzbarsky]]

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

Comment 10

[Justin Lebar (not reading bugmail)]

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

Comment 11

[Boris Zbarsky [:bzbarsky]]

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

Comment 12

[Justin Lebar (not reading bugmail)]

Attachment: Dispatch the hashchange event synchronously.

Comment 13

[Justin Lebar (not reading bugmail)]

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

Comment 14

[Justin Lebar (not reading bugmail)]

>  NS_ASSERTION(nsContentUtils::IsSafeToRunScript(),
>               "Must be safe to run script here.");

Should this go in DispatchAsyncHashchange() or in FireHashchange()?

Comment 15

[Justin Lebar (not reading bugmail)]

Attachment: Patch v2: Adding test fix.

Comment 16

[Justin Lebar (not reading bugmail)]

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 17

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

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?

Comment 18

[Justin Lebar (not reading bugmail)]

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

Comment 19

[Justin Lebar (not reading bugmail)]

Attachment: Patch v3

Comment 20

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

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

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

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.

Comment 22

[Boris Zbarsky [:bzbarsky]]

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.

Comment 23

[Boris Zbarsky [:bzbarsky]]

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.

Comment 24

[Benjamin Smedberg]

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

Comment 25

[Justin Lebar (not reading bugmail)]

Attachment: Patch v4 - No IID change

Comment 26

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

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

Comment 27

[Justin Lebar (not reading bugmail)]

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

Comment 28

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

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

Comment 29

[Justin Lebar (not reading bugmail)]

http://hg.mozilla.org/mozilla-central/rev/50eb584944aa

Comment 30

[Justin Lebar (not reading bugmail)]

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

Comment 31

[[not reading bugmail]]

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