494543 - Can't add items to Dell.com shopping cart

Status: VERIFIED FIXED

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

Description

[Noah (NoahSUMO/SolidSnake) [:Noah]]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20090410 Firefox/3.0
Build Identifier: 

Using the url above, wait for the components animation to finish building, click 'add to shopping cart' next to 'add to wishlist'. Notice nothing happens. Mouseover the 'cart' text at the top of the page, you'll see 'your cart is empty'.

Error console produces:
Error: uncaught exception: [Exception... "Security error" code: "1000" nsresult: "0x805303e8 (NS_ERROR_DOM_SECURITY_ERR)" location: "http://i.dell.com/images/global/js/s_code_dell_vh19_3.js Line: 647"]

Regression range:
4/10 - works
4/11 - broken

JIT is not a factor since I permanently disabled it. Tested with cookies & 3rd party cookies on.

Reproducible: Always

Comment 1

[Mike Shaver (:shaver emeritus)]

Can you reproduce with a 1.9.1/FF3.5 nightly?  Might be wrapper stuff that hasn't made it to branch yet (I sure am hoping!)

Comment 2

[Ria Klaassen (not reading all bugmail)]

I get this as regression range: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=fef8eb29d1e5&tochange=f1c65acaccf3 which points to Bug 431819 (and is different from Noah's). For me it broke only a few days ago.
And it is indeed reproducible in Shiretoko. :P

Comment 3

[Mike Shaver (:shaver emeritus)]

Breaking a few days ago is consistent with my experience shopping on Dell with Shiretoko last week, at least.  I wouldn't have expected either of those changes to have this effect, but maybe Dell uses DOM storage and we boned it?

Comment 4

[Mike Shaver (:shaver emeritus)]

Can we get a 1.9.1-stream regression range?

Comment 5

[Mike Shaver (:shaver emeritus)]

Alas, hourly-archive.localgho.st doesn't have any 1.9.1 hourlies between 16 and 22 May. :(

Comment 6

[Andreas Gal :gal]

With a 3 day old build I wasn't able to see some of the text on bestbuy.com in their shopping cart. Updating to TM tip fixed that. Might be related. I suggest verifying with TM tip before bisecting.

Comment 7

[Mike Shaver (:shaver emeritus)]

This was never broken on tracemonkey: both may 18 and latest TM builds work, likely because we haven't yet merged the offending m-c checkin back to TM.  I've got a feeling in my gut about the DOM storage changes...

Comment 8

[Mike Shaver (:shaver emeritus)]

(Also, I think Noah's initial regression range is just wrong, sorry.)

Comment 9

[Mike Shaver (:shaver emeritus)]

Moving to DOM, copying author and reviewer of likely-regressing bug 455070.

jst or honza, can you figure this out or back it out of 1.9.1 ASAP?

Comment 10

[Mike Shaver (:shaver emeritus)]

WFM now -- wonder if they updated the site to fix use of the old API?  That would be...sort of rad, sort of scary.

Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.1pre) Gecko/20090522 Shiretoko/3.5pre (.NET CLR 3.5.30729)

Comment 11

[Mike Shaver (:shaver emeritus)]

You'd think I knew how to use bugzilla by now -- this is apparently still broken on m-c, but works on 1.9.1 per above.  Andrei will confirm.

Comment 12

[Ria Klaassen (not reading all bugmail)]

Sad to say that it is still failing here with the latest Shiretoko hourly on Windows.

Comment 13

[Andrei Hajdukewycz [:sancus]]

It is definitely still failing in 1.9.1 with the latest nightly, however there is some kind of inconsistency that causes it to work sometimes. It's not just uncleared cookies, I tested that, so not completely sure. This is also making it a bit frustrating to figure out what the regrange actually is.

Comment 14

[Noah (NoahSUMO/SolidSnake) [:Noah]]

Mike, first of all, I do my homework. I double checked each build (just because I'm skeptical in general), and by that I mean I emptied out the profile contents after each build leaving just the prefs.js (which has PB mode permanently set so cache is never saved locally, just to memory, so I can rule out any stale cache possibilities) and ran each build twice using my test link.

I tested again tonight. 4/10 works everytime. 4/11 consistently fails. Short of me producing screencasts with the About dialog open, which I can do I'm just short of HD space at the moment. Can you actually test the 2 trunk builds I pointed out? It sounds like you just skimmed the changelog between these 2 trunk builds and deemed my regression range flawed just because the checkins looked harmless to you. You know better than I, any patch can cause odd things to happen. Can you back out some DOM or JS bugs in that period (like bug 487570 that is supposedly mac only but could be All OSes b/c we don't feel like updating it to All & leave it on either Lin or Mac) and make a tryserver build for us?

Andrei, do you have JIT enabled? Because that's been consistently disabled in my tests. It could be the difference. It also could be that I surf in permanent PB mode as well.

Ria, not sure how you got a different range. Maybe our differences are due to the ones I pointed out to Andrei.

Comment 15

[Michael Ryan]

Definitely something going on with sessionStorage. Try this:

javascript:try{sessionStorage;}catch(e){alert(e);}void(0);

That is supposed to do nothing. Seemingly every page on dell.com throws the security error.

Comment 16

[Honza Bambas (:mayhemer)]

Going to take a look at this right now.

Comment 17

[Honza Bambas (:mayhemer)]

In nsPrincipal::Subsumes we do nsPrincipal::Equals that calls nsScriptSecurityManager::CheckSameOriginPrincipal. SecurityCompareURIs fails because we compare subject's "dell.com" against object's "configure.us.dell.com", page changes it's domain.

The page changes it's domain to "dell.com" after it does the BUILD CHECK first (it takes time). This assigns to subject's principal mDomain = "http://dell.com/...". While accessing the storage (when the "Add to cart" button is pressed) we check the subject principal subsumes the object principal. But the problem is that object's origin in this case is "configure.us.dell.com" because sessionStorage object was created for the page sooner than the page changed it's domain.

The creator of sessionStorage object is session store system that saves the sessionStorage content in "timer-callback" notification (it is cause of randomness of fault reproduction!). This unfortunately also means creation of the sesisonStorage object even it is empty (doesn't exist at that moment) and assigning "configure.us.dell.com" to the storage's object (mPrincipal). 

We may change the session store system to not to create the sessionStorage object when it has not already been created by the content. This means publishing a method of nsDocShell taking aCreate parameter (we already have such, but it's just a private method). On 1.9.1 this means new interface or change of one added quit recently and dedicated to session storage.

I was testing with m-c tip. Definitelly a regression from bug 455070, we changed the way of security checking.

Will check with 1.9.1 tip and create a patch for this soon.

Comment 18

[Ria Klaassen (not reading all bugmail)]

Noah is correct, and that is the big trap for it is impossible to test all 360 nightlies. The problem look exactly the same but the error is slightly different and the bug has only existed a few days.

Comment 19

[Honza Bambas (:mayhemer)]

(In reply to comment #18)
> The problem look exactly the same but the error is slightly
> different and the bug has only existed a few days.

Can you explain please?

Comment 20

[Ria Klaassen (not reading all bugmail)]

The error WAS:
 
Error: uncaught exception: [Exception... "Cannot find interface information for parameter arg 0 [nsIDOMWindowInternal.pkcs11]"  nsresult: "0x80570006 (NS_ERROR_XPC_CANT_GET_PARAM_IFACE_INFO)"  location: "JS frame :: http://i.dell.com/images/global/js/s_code_dell_vh19_3.js :: anonymous :: line 647"  data: no]

"Fixed" range: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=71b36d722fbb&tochange=342e2383009f

Comment 21

[Honza Bambas (:mayhemer)]

Attachment: v1 for trunk

Patch for the trunk, no need to modify any interfaces, same for 1.9.1.

Comment 22

[Honza Bambas (:mayhemer)]

Attachment: v1 for 1.9.1

Same approach, merged for 1.9.1 branch.

Comment 23

[Mike Shaver (:shaver emeritus)]

Noah: apologies; my statement wasn't because I dismissed your checkin range's contents, but because I had multiple later builds that worked.  It might indeed have been a different failure earlier, as Ria suggests.

Comment 24

[Honza Bambas (:mayhemer)]

Shiretoko try builds with the 1.9.1 patch: https://build.mozilla.org/tryserver-builds/[e-mail]-dellcart/

Feel free to test.

Comment 25

[Honza Bambas (:mayhemer)]

Just a note that the patch is more a workaround then a true fix. We should probably think of changing the code of nsDOMStorage2::CanAccess, replace principal::Subsumes with something else?

Comment 26

[Robert Sayre]

Needs a test.

Comment 27

[Brendan Eich [:brendan]]

Any CAPS or WHAT-WG/HTML5 followup bugs to file?

/be

Comment 28

[Marcia Knous [:marcia]]

Using the tryserver build in Comment 24, I am able to add the configuration in the URL to my cart.  Build ID is: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1pre) Gecko/20090524 Shiretoko/3.5pre.

Comment 29

[Simon Bünzli]

Comment on attachment 379420 [details] [diff] [review]
v1 for 1.9.1

A few drive-by comments:

>         let docShell_191 = aDocShell.QueryInterface(Ci.nsIDocShell_MOZILLA_1_9_1_SessionStorage);

There's no need to get the interface in a new variable. aDocShell.QueryInterface(...) makes the interface available for aDocShell itself. Also, instead of repeatedly doing this in a loop, why not just do it once at the top of the function?

>+        storage = docShell_191.getSessionStorageForPrincipal(principal, false);

Does this mean that getSessionStorageForURI is broken while getSessionStorageForPrincipal isn't? If so, shouldn't you at least document this in the IDL and on dev.m.o? Also, please add a comment to the Trunk patch citing the reason for not using getSessionStorageForURI and a bug number.

>+        if (storage)
>+          storageItemCount = storage.length;

On a side note: This'll also fix bug 491083.

>       catch (ex) { /* sessionStorage might throw if it's turned off, see bug 458954 */ }

BTW: Does this comment still hold with all the late changes to sessionStorage?

Comment 30

[Honza Bambas (:mayhemer)]

Attachment: v1.1 for trunk

(In reply to comment #29)
> (From update of attachment 379420 [details] [diff] [review])
> A few drive-by comments:
> 
> >         let docShell_191 = aDocShell.QueryInterface(Ci.nsIDocShell_MOZILLA_1_9_1_SessionStorage);
> 
> There's no need to get the interface in a new variable.
> aDocShell.QueryInterface(...) makes the interface available for aDocShell
> itself. Also, instead of repeatedly doing this in a loop, why not just do it
> once at the top of the function?
> 
Done

> >+        storage = docShell_191.getSessionStorageForPrincipal(principal, false);
> 
> Does this mean that getSessionStorageForURI is broken while
> getSessionStorageForPrincipal isn't? If so, shouldn't you at least document
> this in the IDL and on dev.m.o? Also, please add a comment to the Trunk patch
> citing the reason for not using getSessionStorageForURI and a bug number.
> 
I am using getSessionStorageForPrincipal only because I can pass parameter aCreate = false to avoid creation of the sessionStorage object. This way I don't need to modify any interfaces on 1.9.1. getSessionStorageForURI implementation does exactly the same as the JS code I provide here - gets principal for the URI and calls getSessionStorageForPrincipal.

> >+        if (storage)
> >+          storageItemCount = storage.length;
> 
> On a side note: This'll also fix bug 491083.
> 
> >       catch (ex) { /* sessionStorage might throw if it's turned off, see bug 458954 */ }
> 
> BTW: Does this comment still hold with all the late changes to sessionStorage?

Hmm... Not sure it fixes that bug or that we are not getting exceptions. I believe not, but I have to check this again. It doesn't belong to this bug anyway.

Comment 31

[Honza Bambas (:mayhemer)]

Attachment: v1.1 for 1.9.1

Comment 32

[Honza Bambas (:mayhemer)]

This uncovers another issue: when a page changes it's domain and then creates sessionStorage we cannot access its sessionStorage object in session store code. We end up with the same exception as we got on the page because URI principal is for "configure.us.dell.com" but sessionStorage principal we compare against is for "dell.com".

Comment 33

[Mike Shaver (:shaver emeritus)]

Honza: does comment 32 indicate another regression due to the late-landing session store patch, or an existing problem?

Comment 35

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

(In reply to comment #33)
> Honza: does comment 32 indicate another regression due to the late-landing
> session store patch, or an existing problem?

From what I understand (Honza to confirm) it's more about us properly supporting how the spec says sessionStorage should work:

http://www.w3.org/TR/webstorage/#dom-sessionstorage

If I understand what Honza's saying, it's that when dell.com creates an object, it's "origin" is set to dell.com, but then the session moves over to configure.us.dell.com we fail the same-origin check ... I'm not sure where the definition of same-origin checking is, so I can't tell if this is Dell misinterpreting how they can use sessionStorage or us being too strict with that check.

Comment 36

[Honza Bambas (:mayhemer)]

(In reply to comment #33)
> Honza: does comment 32 indicate another regression due to the late-landing
> session store patch, or an existing problem?

I'm afraid it's a regression. And there is one more issue: we broke https pages with it. The principal in sessionStorage contains a certificate, but the URI codebase principal we create in session store code is just containing the URI (no certificate). Security check with nsIPrincipal.subsumes (that means .equals in the most common case of nsPrincipal implementation) won't pass.

But I'd say it's an old bug in DOM storage code. I'd say that here http://mxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#2197 should not be tested aPrincipal but the subject principal (the caller). That would be IMO correct (and make more sense) and solve all problems with session store code.

Comment 37

[Honza Bambas (:mayhemer)]

(In reply to comment #35)
Check bug 494799.

Comment 38

[Boris Zbarsky [:bzbarsky]]

That patch won't save session storage correctly on pages where the principal doesn't match the URI (about:blank, javascript:, data:, generated with document.write, uses a signed cert, etc, etc), as far as I can tell.  Is that the intended effect?

Comment 39

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

Isn't that true of the existing code too, though, per comment 30 paragraph 2?

Comment 40

[Boris Zbarsky [:bzbarsky]]

Oh, I see; I missed that part somehow.  Given that, patch is fine by me.

We should probably spin off a separate bug on properly using this shiny new API that doesn't assume things that are false about how URIs and principals are related.

Comment 41

[Honza Bambas (:mayhemer)]

(In reply to comment #36)
> But I'd say it's an old bug in DOM storage code. I'd say that here
> http://mxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#2197
> should NOT be tested aPrincipal but the subject principal (the caller). That
> would be IMO correct (and make more sense) and solve all problems with session
> store code.

Any opinions on this proposal?

Comment 42

[Boris Zbarsky [:bzbarsky]]

The test there is ensuring that we got a storage for the correct principal.  So it is, in fact, correct, as far as I can see.  Using the subject principal wouldn't tell you much, I don't think; and in particular it would fail miserably for any C++ consumer.

Comment 43

[Dietrich Ayala (:dietrich)]

Comment on attachment 379531 [details] [diff] [review]
v1.1 for trunk


>+        // Using getSessionStorageForPrincipal instead of getSessionStorageForURI
>+        // just to be able to pass aCreate = false, that avoids creation of the
>+        // sesisonStorage object for the page earlier than the page really

s/sesison/session/

r=me

Comment 44

[Dietrich Ayala (:dietrich)]

nit fixed, pushed to trunk: http://hg.mozilla.org/mozilla-central/rev/4b340dd7cd87

Comment 45

[Noah (NoahSUMO/SolidSnake) [:Noah]]

One more clarification:

4/11-4/15 builds produced this error:
Error: uncaught exception: [Exception... "Cannot find interface information for
parameter arg 0 [nsIDOMWindowInternal.pkcs11]"  nsresult: "0x80570006
(NS_ERROR_XPC_CANT_GET_PARAM_IFACE_INFO)"  location: "JS frame ::
http://i.dell.com/images/global/js/s_code_dell_vh19_3.js :: anonymous :: line
647"  data: no]

Starting from 4/16 & after, it changed to:
Error: uncaught exception: [Exception... "Security error" code: "1000"
nsresult: "0x805303e8 (NS_ERROR_DOM_SECURITY_ERR)" location:
"http://i.dell.com/images/global/js/s_code_dell_vh19_3.js Line: 647"]

Comment 46

[Honza Bambas (:mayhemer)]

Comment on attachment 379531 [details] [diff] [review]
v1.1 for trunk

After we land patch for bug 495112 we don't need this patch.

Comment 47

[Honza Bambas (:mayhemer)]

Comment on attachment 379532 [details] [diff] [review]
v1.1 for 1.9.1

Sorry, THIS patch.

Comment 48

[Dietrich Ayala (:dietrich)]

(In reply to comment #46)
> (From update of attachment 379531 [details] [diff] [review])
> After we land patch for bug 495112 we don't need this patch.

hrm, i didn't see these updates until just now after i pushed to branch:

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/59210d5fd692

from the comments, that bug doesn't seem near finished, so just back these changes out as part of that bug, if/when it lands.

Comment 50

[Marcia Knous [:marcia]]

Verified fixed on the 1.9.1 branch using Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1pre) Gecko/20090528 Shiretoko/3.5pre and Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1pre) Gecko/20090528 Shiretoko/3.5pre.

I verified using the URL provided in the URL field.

Thanks Noah for catching this bug before we shipped and making the e-commerce experience better for everyone.

Comment 51

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Verified on trunk too with builds on OS X and Windows like Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090529 Minefield/3.6a1pre ID:20090529031523

Can we get this into the test-suite?