492219 - localStorage's constructor should be Storage

Status: VERIFIED FIXED

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

Description

[Eli Grey (:sephr)]

User-Agent:       Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.0.10) Gecko/2009042523 Ubuntu/9.04 (jaunty) Firefox/3.0.10 GTB5
Build Identifier: 

localStorage currently has the constructor, Storage2 while globalStorage items currently have Storage as a constructor. This would break any webapps that extend Storage.prototype to extend globalStorage items AND localStorage for WebKit.

I suggest making globalStorage items and localStorage both have same constructor.

Reproducible: Always

Steps to Reproduce:
1. Storage.prototype.foo = function() { return 123; }
2. localStorage.foo()
3. globalStorage[location.hostname].foo()
Actual Results:  
localStorage.foo() throws and error and globalStorage[location.hostname].foo() returns 123.

Expected Results:  
localStorage.foo() and globalStorage[location.hostname].foo() both return 123.

Comment 1

[Eli Grey (:sephr)]

Attachment: testcase

Comment 2

[Eli Grey (:sephr)]

This bug breaks the Noteboard webapp at noteboard.eligrey.com. Line 188 @ http://noteboard.eligrey.com/noteboard.full.js defines Storage.prototype.itemExists expecting it to be accessible via localStorage.itemExits.

For Safari and Firefox <3.5 this works fine. If the script detects globalStorage but no localStorage, it defines localStorage as globalStorage[location.hostname]. Due to Firefox >=3.5 not inheriting the properties set on Storage.prototype, the webapp doesn't work any more.

I have discontinued development of Noteboard a while back due to it not being that useful, but Google Analytics reports to me that there are still quite a few people who use this webapp daily.

Comment 3

[Honza Bambas (:mayhemer)]

Suggestion: drop globalStorage or at least stop supporting its compatibility. It was already drop from the spec anyway.

To explain why there is Storage2: globalStorage items are using an old interface (nsIDOMStorage) that returns nsIDOMStorageItem for each key in the storage (when using getItem), it has attributes 'value' and 'secure'. But, localStorage was re-specified to return the value for a key directly (so, it returns DOMString for each key in the storage). localStorage (and sessionStorage will be, bug 455070) is implemented by a different C++ class then globalStorage items. This was necessary to let XPConnect correctly distinguish between the two interfaces (a single class implementing both interfaces didn't work - we was not able to control which interface is exposed to scripts, getItem methods on both interfaces had the same name but a different signature). To achieve it, there are Storage and Storage2 DOM class implementations. We have NS_DEFINE_CLASSINFO_DATA(Storage, nsStorageSH) for globalStorage items and NS_DEFINE_CLASSINFO_DATA(Storage2, nsStorage2SH) for localStorage.

I'd rename Storage to StorageObsolete or something like that and Storage2 to Storage. This should be IMO fixed after bug 455070 because it would badly effect sessionStorage.

Opinions?

Comment 4

[Johnny Stenback (:jst)]

Blocking, and I think the rename of Storage to StorageObsolete etc is the way to go here. The sooner we do this the better.

Comment 5

[Honza Bambas (:mayhemer)]

Attachment: v1

Storage -> StorageObsolete (affects globalStorage items)
Storage2 -> Storage (now localStorage's constructor is Storage)

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I pushed this:
http://hg.mozilla.org/mozilla-central/rev/0602828f3742
and backed it out:
http://hg.mozilla.org/mozilla-central/rev/bbc4bb97f00a
due to assertions firing on the leak boxes, where they're fatal:
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1242360246.1242362838.13699.gz
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1242360457.1242361793.11953.gz

Comment 7

[Honza Bambas (:mayhemer)]

This looks bad. The assertion checks that a dom class name (e.g. Storage) is the name of an interface we demand to use (e.g. nsIDOMStorage); a part w/o the "nsIDOM" is compared. This means API change, we must rename the interface. jst or someone else should approve such a change.

Comment 8

[Honza Bambas (:mayhemer)]

As this will effect sessionStorage too, we should also consider getting bug 455070 to 1.9.1. It's waiting for months for a regular review...

Comment 9

[Honza Bambas (:mayhemer)]

Attachment: v2 [Checkin comment 11]

Global replace patch. Interfaces are renamed and both their iids were changed.

Comment 10

[Johnny Stenback (:jst)]

Comment on attachment 377668 [details] [diff] [review]
v2 [Checkin comment 11]

Looks good.

Comment 11

[Honza Bambas (:mayhemer)]

Comment on attachment 377668 [details] [diff] [review]
v2 [Checkin comment 11]

http://hg.mozilla.org/mozilla-central/rev/8208b02cba3c

Comment 12

[Boris Zbarsky [:bzbarsky]]

Why no iid rev on nsIDocShell here?  I guess existing C++ code would just keep working, since nothing other than interface names changed?  But it still seems more correct to rev the iid to me...

And how do you plan to do the equivalent change on 1.9.1 where you can't change nsIDocShell?  Or is this change compatible enough that we can do it there?

Comment 13

[Honza Bambas (:mayhemer)]

(In reply to comment #12)
> Why no iid rev on nsIDocShell here?  I guess existing C++ code would just keep
> working, since nothing other than interface names changed?  But it still seems
> more correct to rev the iid to me...
> 
> And how do you plan to do the equivalent change on 1.9.1 where you can't change
> nsIDocShell?  Or is this change compatible enough that we can do it there?

It is just a rename, unrecompiled C code will be ok with it from the binary point of view. But what's wrong is that doing QI to nsIDOMStorage or nsIDOMStorageObsolete fails in such an unrecompiled code - iid has changed to new ones in both cases.

So, my solution for 1.9.1 is NOT TO CHANGE iid of the renamed interfaces, so there will be no iid change in the whole patch, unrecompiled code will be happy.

When binary extension developers will recompile with this patch, their build will break and they have to fix manually. Probably start a thread about this on dev.platform?

For the trunk only we should probably have a fix to change iids of all remaining affected interfaces (nsIDocShell.idl nsIDOMStorageList.idl nsIDOMStorageManager.idl nsIDOMStorageWindow.idl)

Reopening, there is apparently still a work to do.

Comment 14

[Honza Bambas (:mayhemer)]

Attachment: trunk-only fix, iid update, v1 [Checkin comment 18]

Updating iids of all affected interface. This patch is intended to land only on trunk.

Comment 15

[Honza Bambas (:mayhemer)]

Attachment: v1 for 1.9.1

Ok, in this patch nsIDOMStorageObsolete has the same iid as nsIDOMStorage had before. Same applies to the new nsIDOMStorage and nsIDOMStorage2. If you agree on this change we can take it to 1.9.1. If not, my experience is not that good to find a different solution at the moment.

Comment 16

[Boris Zbarsky [:bzbarsky]]

If the only issue here is the naming of the relevant prototype in the DOM, then for branch it might make sense to leave all the IDL as-is, having C++ consumers deal with the difference in naming between C++ and the DOM but not having any changes from the current setup in terms of code they write, and changing the naming in the DOM by just changing

  NS_DEFINE_CLASSINFO_DATA(Storage, nsStorageSH, flags)

to

  NS_DEFINE_CLASSINFO_DATA_WITH_NAME(Storage, StorageObsolete, nsStorageSH,
                                     flags)

in nsDOMClassInfo.cpp.  And similarly for Storage2, using Storage as the name.  Is there some obvious issue left that that wouldn't resolve as far as branch is concerned?

Comment 17

[Honza Bambas (:mayhemer)]

(In reply to comment #16)
> If the only issue here is the naming of the relevant prototype in the DOM, then
> for branch it might make sense to leave all the IDL as-is, having C++ consumers
> deal with the difference in naming between C++ and the DOM but not having any
> changes from the current setup in terms of code they write, and changing the
> naming in the DOM by just changing
> 
>   NS_DEFINE_CLASSINFO_DATA(Storage, nsStorageSH, flags)
> 
> to
> 
>   NS_DEFINE_CLASSINFO_DATA_WITH_NAME(Storage, StorageObsolete, nsStorageSH,
>                                      flags)
> 
> in nsDOMClassInfo.cpp.  And similarly for Storage2, using Storage as the name. 
> Is there some obvious issue left that that wouldn't resolve as far as branch is
> concerned?

It doesn't work. With patch like this:

-  NS_DEFINE_CLASSINFO_DATA(Storage, nsStorageSH,
+  NS_DEFINE_CLASSINFO_DATA_WITH_NAME(Storage, StorageObsolete, nsStorageSH,

-  NS_DEFINE_CLASSINFO_DATA(Storage2, nsStorage2SH,
+  NS_DEFINE_CLASSINFO_DATA_WITH_NAME(Storage2, Storage, nsStorage2SH,

I got the constructor I want but it still fails on the assertion here:
NS_ASSERTION(nsCRT::strcmp(CutPrefix(name), mData->mName) == 0, "...")

Comment 18

[Honza Bambas (:mayhemer)]

Comment on attachment 378008 [details] [diff] [review]
trunk-only fix, iid update, v1 [Checkin comment 18]

http://hg.mozilla.org/mozilla-central/rev/a025affbfd4f

Comment 19

[Honza Bambas (:mayhemer)]

All work for the trunk has been done. If we don't agree on the 1.9.1 patch then it seems good to open a new bug.

Comment 20

[Boris Zbarsky [:bzbarsky]]

You probably also need to use DOM_CLASSINFO_MAP_BEGIN_NO_CLASS_IF instead of DOM_CLASSINFO_MAP_BEGIN for those classes.   Double-check with jst, though?  I _think_ that should do the right thing, but he'd know for sure.

You can take whatever approach for 1.9.1 you prefer; if you prefer to spin off a separate bug, please do.

Comment 21

[Honza Bambas (:mayhemer)]

Attachment: v1 for 1.9.1 [Checkin comment 23]

Successfully tested, fixes the problem w/o any moves with interfaces.

Comment 22

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 378575 [details] [diff] [review]
v1 for 1.9.1 [Checkin comment 23]

Looks great.

Comment 23

[Honza Bambas (:mayhemer)]

Comment on attachment 378575 [details] [diff] [review]
v1 for 1.9.1 [Checkin comment 23]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/d592efbf197c

Comment 24

[Aakash Desai [:aakashd]]

added to mochiTest, verified FIXED on builds using the attached testcase:

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1pre) Gecko/20090602 Shiretoko/3.5pre ID:20090602044233

and

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20090601 Minefield/3.6a1pre ID:20090601041706