Closed Bug 770844 Opened 12 years ago Closed 12 years ago

Reintroduce window.mozIndexedDB

Categories

(Core :: Storage: IndexedDB, defect)

Product:
Core
Component:
Storage: IndexedDB
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla20
Tracking Flags:
Tracking Status
firefox15 --- unaffected
firefox16 + fixed
firefox17 + fixed
firefox18 + fixed
firefox19 --- fixed

People

(Reporter: daleharvey, Assigned: khuey)

References

Details

(Keywords: dev-doc-complete)

Attachments

(1 file)

Bandaid for beta
1.54 KB, patch
sicking
: review+
bajaj
: approval-mozilla-aurora+
akeybl
: approval-mozilla-beta+
Details | Diff | Splinter Review 
Since we unprefixed indexedDB, I have seen a few occurrences of code that does

var indexedbDB = window.indexedDB || window.webkitIndexedDB || window.moz .....

in the global scope, this will cause indexedDB and window.indexedDB to be undefined (in firefox and opera, chrome doesnt allow hoisting to override it)

All of the properties on window. have the same bevaviour (except for window.document)

If we just attempt to write window.indexedDB = 'foo' it cant be overwritten, I wouldnt expect hoisting to be able to.

The initial code snippet is given by pretty much every tutorial to indexedDB, so I expect a lot of code to break in firefox (and not chrome) unless overwriting it is protected against.
Assignee: nobody → general
Status: NEW → UNCONFIRMED
Component: DOM: IndexedDB → JavaScript Engine
Ever confirmed: false
OS: Mac OS X → All
QA Contact: indexeddb → general
Hardware: x86 → All
Version: unspecified → Trunk
> The initial code snippet is given by pretty much every tutorial to indexedDB,

Er, that's really bad.  The behavior we have is the one the spec requires; Chrome is violating it (by putting IDL properties on the object instead of the prototype).

We should probably evangelize the tutorials at the very least.  Links, please?
One more thing.  As a JS engine bug, this bug is invalid.  But I'm not sure whether the move into this component was even right... Maybe this bug is about introducing some sort of spec violation for indexedDB specifically (and escalating it to a spec issue, of course).
By the way, the correct way to do this is:

(function() {
  var indexedDB = window.indexedDB || window.webkitIndexedDB || window.moz...
  window.indexedDB = indexedDB;
})();

And this part from comment 0:

> If we just attempt to write window.indexedDB = 'foo' it cant be overwritten, I wouldnt
> expect hoisting to be able to.

Is wrong.  Per ES spec, variable declaration and assignment behave differently wrt shadowing prototype properties of the global.  Again, the difference with Chrome is that the property is not on the prototype to start with.
Link to where the previous code snippet is used

http://www.html5rocks.com/en/tutorials/indexeddb/todo/
https://developer.mozilla.org/en/IndexedDB/IndexedDB_primer
http://hacks.mozilla.org/2012/02/storing-images-and-files-in-indexeddb/

And yes I knew it wouldnt be a jsengine bug, I wasnt sure if there was scope in the spec / whether it would be worth it to correct the different behaviours between us / chrome though.
Well, at some point presumably Chrome will fix their IDL property handling.  ;)

The examples you link to are a bit insiduous because they're totally fine inside a function.  It's just at global scope that they're a problem....

I've fixed the devmo article and set mail to Robert about the hacks.mozilla.org article.

Dale, I see you already commented on the html5rocks article.  I sent a tweet to the article's author and also pointed our developer engagement folks at that article.  Here's hoping it will get fixed.
An even simpler snipped that might be easier to evangelize and works in all contexts is
window.indexedDB = window.indexedDB || window.webkitIndexedDB || window.moz...
Ah, yes, nice.  Updated devmo.
(In reply to Till Schneidereit [:till] from comment #7)
> An even simpler snipped that might be easier to evangelize and works in all
> contexts is
> window.indexedDB = window.indexedDB || window.webkitIndexedDB ||
> window.moz...

Assuming that, DOM-wise, Window.prototype should have a descriptor like { get: getIndexedDB, set: undefined, enumerable: true, configurable: true } on it, that doesn't work in strict mode code, because [[CanPut]] returns false if it sees an accessor property with an undefined setter.
But in that case, shouldn't it simply do nothing, making window.indexedDB be the correct value by default?

I suppose that that means that the following should work as well:
window.indexedDB = window.webkitIndexedDB || window.moz...

But that relies on the additional assumption that all environments make windows.indexedDB read-only.


Maybe I'm misunderstanding and you're thinking about use-cases such as
var usedDB = (window.indexedDB = window.indexedDB || window.webkitIndexedDB || window.moz...)

If so: well, I sure hope that that's not a pattern anyone would teach. Or use.
(In reply to Till Schneidereit [:till] from comment #10)
> But in that case, shouldn't it simply do nothing, making window.indexedDB be
> the correct value by default?

unless the script is in strict mode, and the absence of the getter throws an exception, I guess. Better could be

if (!("indexedDB" in window)) {
  window.indexedDB = ...;
}
> unless the script is in strict mode, and the absence of the getter throws an
> exception, I guess. Better could be

Right, thanks.

TIL that the scratchpad doesn't respect "use strict", even in functions.
> TIL that the scratchpad doesn't respect "use strict", even in functions.

That doesn't sound right.  File a new bug?
> that doesn't work in strict mode code

None of the options suggested really work in strict mode.  That's just life.
(In reply to Nicholas Nethercote [:njn] from comment #13)
> > TIL that the scratchpad doesn't respect "use strict", even in functions.
> 
> That doesn't sound right.  File a new bug?

Already did: bug 771936
Should this bug be confirmed and maybe even assigned?

/be
(In reply to Brendan Eich [:brendan] from comment #16)
> Should this bug be confirmed and maybe even assigned?

What's the bug? Are you suggesting that we imitate the Chrome behavior for 'indexedDb'? Or maybe we should file a bug with them to resolve it?
Unless I’m misunderstanding something, this is why http://mathias.html5.org/specs/javascript/#top-level-var-statements says:

> The erratum in https://bugs.ecmascript.org/show_bug.cgi?id=78#c0 must be followed so that `var` statements at the top level of scripts can shadow any properties from the global object’s prototype chain.
Tracking for 16 given we unprefixed indexeddb in 16 - my assumption is that this will either end up being a TE bug or something we plan to fix in-product.
tracking-firefox16: ? → +
(In reply to Alex Keybl [:akeybl] from comment #19)
> Tracking for 16 given we unprefixed indexeddb in 16 - my assumption is that
> this will either end up being a TE bug or something we plan to fix
> in-product.

It sounds like a TE bug to me. Does anyone think we should do something specific?
The alternatives are:
* Treat it as TE
* Put the indexedDB getter on the global object itself
  rather than on the proto chain
* Make 'var' in this case (for some definition of 'this
  case') not set a property on the global object if a
  property descriptor exists in the proto chain.
(In reply to Jonas Sicking (:sicking) from comment #21)
> The alternatives are:
> * Treat it as TE
> * Put the indexedDB getter on the global object itself
>   rather than on the proto chain
> * Make 'var' in this case (for some definition of 'this
>   case') not set a property on the global object if a
>   property descriptor exists in the proto chain.

Which do you recommend?
I recommend TE until we hear of real-life breakage, at least.
(In reply to :Ms2ger (Back and backlogged) from comment #23)
> I recommend TE until we hear of real-life breakage, at least.

As we see over and over again , a lack of feedback pre-release does not necessarily mean there isn't website breakage. We should be trying to prevent chemspills as much as possible.

Comment 0's assertion that the "initial code snippet is given by pretty much every tutorial to indexedDB, so I expect a lot of code to break in firefox (and not chrome) unless overwriting it is protected against" is what concerns me the most.

How difficult would the other two options be to implement? If there's reason to believe we're overrotating on this issue, please let us know why. Thanks!
Option 2 is not hard to implement, but is a clear spec violation....
I'm with Ms2ger that it's better to keep to the spec until forced to break it. To me, the scenario in comment 0 is hypothetical, so I'm not that worried about it. But that's just my guesswork opinion.
The scenario in comment 0 is basically happening in every single webpage that uses indexedDB right now.

So it's unfortunately not hypothetical.
Happens at global scope?  That part is important....
Yes, all the tutorials I've seen has done it in global scope. The point of that line is to provide a "polyfill" which makes prefixed implementations appear to be unprefixed which means that you have to run it at global scope.

We certainly have an option to play hardball here and hope that websites will fix their code. IndexedDB is still a new API and so unlikely has a lot of usage out there. But we don't have any data other than that we've heard several times already about sites breaking due to the pattern in comment 0.
Yes at the time every tutorial that I could find used that pattern, html5rocks and the mozilla block still do and I found the bug because it was used all over Mozilla code (in boot2gecko)

The major indexedDB libraries dont use this pattern and I think that is how the vast majority of people will be using indexeddb, so it may not be a major problem, leaving that up to you all.

But it also seems to me that this problem is applicable to more prefixed properties in which this is a common pattern for unprefixing them all.
The fact that we didn't even manage to change our own tutorials over the course of more than three weeks isn't exactly reassuring, IMHO.
Uh... https://hacks.mozilla.org/2012/02/storing-images-and-files-in-indexeddb/ has a clear comment in the first example, which was added the day I made comment 6, explaining that this is only safe to do inside a function, etc.  The complete code has always been inside a function.  So you can be reassured now!

We obviously can't control Google's marketing team (re html5rocks).
It would probably be worth changing that example to just plainly do |window.indexedDB = ...| instead. I don't see much reason to run that code inside a function.
(In reply to Boris Zbarsky (:bz) [In and out Aug 1 - 10, out Aug 11-20] from comment #33)
> Uh...
> https://hacks.mozilla.org/2012/02/storing-images-and-files-in-indexeddb/ has
> a clear comment in the first example, which was added the day I made comment
> 6, explaining that this is only safe to do inside a function, etc.  The
> complete code has always been inside a function.  So you can be reassured
> now!
> 
> We obviously can't control Google's marketing team (re html5rocks).

Ok, so we added a comment - below the wrong code. People don't RTFM, they copy code from tutorials. And if they then successfully test in their browser of choice, which might just happen to be Chrome, they deploy.

So no: I'm not exactly reassured, unfortunately.
For anyone who is not happy: _you_ can mail the article author just as well as _I_ can.  Please feel free to exercise that ability.
(In reply to Boris Zbarsky (:bz) [In and out Aug 1 - 10, out Aug 11-20] from comment #36)
> For anyone who is not happy: _you_ can mail the article author just as well
> as _I_ can.  Please feel free to exercise that ability.

Sorry, you're right. I didn't want to bitch or be snippy.

What I actually wanted to say is that my confidence in the web doing the right thing because it is the right thing is very low. And that's not contingent on us changing a tutorial, so please excuse the red herring.
(In reply to Till Schneidereit [:till] from comment #37)
> (In reply to Boris Zbarsky (:bz) [In and out Aug 1 - 10, out Aug 11-20] from
> comment #36)
> > For anyone who is not happy: _you_ can mail the article author just as well
> > as _I_ can.  Please feel free to exercise that ability.
> 
> Sorry, you're right. I didn't want to bitch or be snippy.

That's fine, and as Boris said: just reach out to any of us in Developer Engagement team if you have any feedback or suggestions.

When I wrote the article, I specifically made sure to clear the code with a couple of Mozilla engineers to ensure it would be correct. However, as has been shown, there is an issue with that approach.

As soon as Boris notified me of the issue at hand I added the comment to the article, and the complete code - both in the complete example and the code at GitHub - is wrapped in a function and is correct.

I do agree, though, that people don't read the manual/comments in the code, and there is a risk of just copy/paste, so I have changed the initial code at the beginning of the article to use window.indexedDB.
(In reply to Jonas Sicking (:sicking) from comment #29)
> Yes, all the tutorials I've seen has done it in global scope. The point of
> that line is to provide a "polyfill" which makes prefixed implementations
> appear to be unprefixed which means that you have to run it at global scope.
> 
> We certainly have an option to play hardball here and hope that websites
> will fix their code. IndexedDB is still a new API and so unlikely has a lot
> of usage out there. But we don't have any data other than that we've heard
> several times already about sites breaking due to the pattern in comment 0.

Given that it's really happening, it seems to me that comes down to how much it would be worth to get browsers to follow the spec vs. how likely it is they actually will vs. how much breakage we have to suffer in the meantime. To (assuredly-not-a-DOM-API-expert) me, exactly which object holds the accessor seems like a detail that isn't really that important, so if there is bustage now, I'd be inclined to move it.
That certainly works for me. I think reducing confusion around this "var shading" is an all around win and moving properties from the prototype chain to the global object accomplishes that.
(In reply to Jonas Sicking (:sicking) from comment #40)
> That certainly works for me. I think reducing confusion around this "var
> shading" is an all around win and moving properties from the prototype chain
> to the global object accomplishes that.

I think that makes this a DOM bug. Feel free to boot this back to the JS component if I'm wrong, or enlist help from us.
Assignee: general → nobody
Status: UNCONFIRMED → NEW
Component: JavaScript Engine → DOM: IndexedDB
Ever confirmed: true
Jonas, are you planning to special-case the indexedDB property or special-case the global?

What does IE implement here?
FWIW I don't have the cycles to do anything here right now due to B2G swampage :(

My suggestion was to put the indexedDB property on the global object itself. Not sure if that counts as "special-case the indexedDB property" or "special-case the global"
Unless you do it with all the other stuff on the Window interface (and possibly EventTarget?), it counts as special-casing indexedDB.

Who does have time to deal with this?  What's needed is:

1)  Testing what IE does.
2)  If we decide to do something here, raising the relevant spec issue(s) (need a way to
    mark these properties in the IDL, for example).  Note that there is already a spec
    mechanism for putting properties directly onto the object: [Unforgeable].  But it
    doesn't sound like we want all the other baggage that brings here.
Since Jonas is swamped with B2G, assigning this to bz for now to continue driving - re-assign to another person as appropriate, we're looking to make sure there are owners for 16 tracked bug at this point in the cycle.
Assignee: nobody → bzbarsky
I'm the wrong person to drive this for two reasons:

1)  I think we should wontfix this bug.
2)  I don't have anyone I could assign to the tasks from comment 44, short of myself.

Punting to someone who might have people to thus assign.
Assignee: bzbarsky → jst
Summary: Javascript Hoisting can override read only window properties → Consider putting indexedDB on the window object itself, not on Window.prototype
And in particular, I wouldn't be able to deal with item 1 from comment 44 myself until after 16 is on beta, because I won't be near my Windows machine until then.
Per bbondy, IE 10 has the indexedDB object on the proto, not on the global.
If these examples are going to be broken in IE too, this should be WONTFIXED, imo.
That's the question: are the examples broken in IE10?  Or are they shipping it prefixed so far?
IE 10 actually has both the prefixed version and the unprefixed version, which eliminates this problem.  We could do the same thing ...
Hrm.  Except we do want to drop prefixes for things long-term, whereas IE may not.  :(
And worse yet, from a web compat perspective this would mean that something like servo has to implement some random prefixed version.
Kyle's figuring out what, if anything, we should do here.
Assignee: jst → khuey
I sent http://lists.w3.org/Archives/Public/public-webapps/2012JulSep/0392.html.

I agree that keeping around a prefixed version is pretty awful.  I think the right action here is probably evangelism.
It's been brought up in the above thread, but I'd actually rather see tutorials evangelize

window.indexedDB = window.indexedDB || window.webkitIndexedDB || window.mozIndexedDB || window.msIndexedDB || ...

Instead of `var indexedDB = ...`. This also addresses the hoisting problem. We're very happy to make that change on html5rocks and think it's quite logical. (Yes, and this has vendor prefix implications that I consider quite reasonable.)

Though I will say, as a developer, this (comment 1) behavior is quite unexpected. Are there many other APIs that exist on the prototype and share this behavior?
Every attribute on the Window interface (just like on every interface) lives on the prototype.
(In reply to Paul Irish from comment #56)
> Though I will say, as a developer, this (comment 1) behavior is quite
> unexpected. Are there many other APIs that exist on the prototype and share
> this behavior?
I think the global object is the least interoperable piece in web browsers. I'm not even sure it is mentionned on WebIDL.
To visualize how the global object looks in different browsers, open http://davidbruant.github.com/ObjectViz/ in different browsers. Top left "flower" is own properties of the global object, then prototype properties, then proto's proto properties and so on. It usually ends with Object.prototype. (if you click, it'll show a click event object)
> I'm not even sure it is mentionned on WebIDL.

It's compeletely specified by the combination of WebIDL and http://www.whatwg.org/specs/web-apps/current-work/multipage/browsers.html#the-window-object for all the non-ES bits.  Now browsers haven't necessarily converged on this yet.
(In reply to Boris Zbarsky (:bz) [In and out Aug 1 - 10, out Aug 11-20] from comment #59)
> > I'm not even sure it is mentionned on WebIDL.
One of these days I should just shut up.

> It's compeletely specified by the combination of WebIDL
I read "For every interface (...) a corresponding property MUST exist on the ECMAScript global object."
But I don't see where it says it should be on the prototype of the global object or as own properties or anything like that.
That' the section that says IndexedDB should be a property on the global.

The part you should be reading is the part that defines the behavior of this IDL

  interface IDBEnvironment {
    readonly attribute IDBFactory indexedDB;
  };
  Window implements IDBEnvironment;

and that's at http://dev.w3.org/2006/webapi/WebIDL/#es-attributes which goes into excruciating detail about what object the property lives on, what the property descriptor looks like, what the getter and setter do when called, etc, etc.
Boris, apologies for going with the "ES5 changed" flow. It did, but ES5.1 reverted per bug 632003 and matching erratum on ES5.

Then some time later, bug 722121 regressed to the broken ES5 state, which is not compatible and not normative.

I filed bug 781739 to get this JS regression fixed, which should fix this bug. Feel free to DUP.

/be
Depends on: 781739
Attached patch Bandaid for beta — — Splinter Review 
For beta, let's do what IE does and have both versions.
Attachment #658608 - Flags: review?(jonas)
Comment on attachment 658608 [details] [diff] [review]
Bandaid for beta

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Unprefixing IndexedDB
User impact if declined: Some code patterns will break.
Testing completed (on m-c, etc.): I tested it manually.
Risk to taking this patch (and alternatives if risky): This is a low-risk alternative to the real fix that is safe for beta.
String or UUID changes made by this patch: It changes the UUID of nsIDOMStorageIndexedDB, but there's no reason for a binary component to use that interface.
Attachment #658608 - Flags: approval-mozilla-beta?
Comment on attachment 658608 [details] [diff] [review]
Bandaid for beta

[Triage Comment]
Approving for Beta to prevent a new IndexedDB regression in FF16.
Attachment #658608 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #66)
> https://hg.mozilla.org/releases/mozilla-beta/rev/977c5986621f

Do we expect to have a fix in the next week or so? If not, let's do the same fix for FF17 as we did for FF16.
tracking-firefox17: ? → +
tracking-firefox18: ? → +
Kyle can you nominate this for Aurora uplift so we can get this in before merge in a little over a week?
s/nominate/prepare a patch for
Comment on attachment 658608 [details] [diff] [review]
Bandaid for beta

I expect that this patch applies to Aurora cleanly or with trivial modifications.

The approval question responses are the same as last time.
Attachment #658608 - Flags: approval-mozilla-aurora?
Attachment #658608 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Ran into this today and filed bug 805794 before being redirected here.
Should this patch have landed on mozilla-central as well?  Fennec nightlies still exhibit the indexedDB aliasing issue.
I should probably just land the bandaid on trunk because I'm not going to get around to actually fixing this anytime soon.
As I understand it, the "real fix" is bug 781739?
Elaborating: my reading of bug 781739 is that the "proper" fix is to make the indexedDB property [[Replaceable]].
No.  The proper fix is probably going to be in this bug and it's to put all Window properties as own properties of the global.
https://hg.mozilla.org/mozilla-central/rev/3766b16a5f9c
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
Updating summary to what actually happened.
Summary: Consider putting indexedDB on the window object itself, not on Window.prototype → Reintroduce window.mozIndexedDB
I repurposed bug 781739 for the real fix here.
Depends on: 975699
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: