906166 - iD editor for OpenStreetMap - deleting a localStorage property sometimes getting error "TypeError: property iD_http://osm.dev_lock is non-configurable and can't be deleted"

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[John Firebaugh]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:26.0) Gecko/20100101 Firefox/26.0 (Beta/Release)
Build ID: 20130816030205

Steps to reproduce:

Occasionally, deleting a localStorage property via a delete expression (rather than via removeItem) results in an error.

For example, sometimes the expression `delete localStorage["iD_http://osm.dev_lock"]` results in the error 'TypeError: property "iD_http://osm.dev_lock" is non-configurable and can't be deleted', while `localStorage.removeItem("iD_http://osm.dev_lock")` always works fine.

Comment 1

[Tiziana Sellitto [:tiziana]]

Thanks for reporting this John. 
Could you provide a minimal testcase or an URL to better understand and reproduce this?

Comment 2

[John Firebaugh]

Unfortunately I was not able to minimize this, but I can tell you that it occurred in the iD editor for OpenStreetMap. The following issues were caused by it, and switching from delete to removeItem solved the issue:

https://github.com/systemed/iD/issues/1691
https://github.com/systemed/iD/issues/1692

Comment 3

[Mariusz Nowak]

I have exactly same problem, and also failed at attempt to produce minimal test case.

It's valid issue.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Is there a non-minimal but _reliable_ testcase?

Comment 5

[Mariusz Nowak]

Boris, try http://ldz.eregistrations.org/ and open console, you'll see "TypeError: property "X" is non-configurable and can't be deleted" errors, and blank page cause of that.

This page worked well on Firefox about 2 weeks ago.

Comment 6

[Till Schneidereit [:till]]

That indeed reproduces reliably, thanks

Comment 7

[Boris Zbarsky [:bzbarsky]]

One-day m-c range: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=3d40d270c031&tochange=c2b375f3a909

Comment 8

[Boris Zbarsky [:bzbarsky]]

Note, of course, that my range is different from "2 weeks ago"..

On the site in comment 5, what I see is that we're in a strict-mode script (specifically http://ldz.eregistrations.org/public.js which has code like this:

	if (userId) localStorage._authenticated = userId;
	else delete localStorage._authenticated;

But if localStorage._authenticated is not actually set (which in my case of course it's not), then what will this do?  Per spec, the object has a named deleter, and this deleter has an identifier and the operation returns void.  So we should land in http://dev.w3.org/2006/webapi/WebIDL/#delete step 2.5 and the delete should never throw.

OK, so what does our implementation do?  This class is still using XPConnect bindings, so we'll call XPC_WN_Helper_DelProperty.  In bug 858677 this function picked up a "JSBool *succeeded" out param that it never sets (red flags should be going off now).  And then the calling code in Interpret() is:

2190     bool succeeded;
2191     if (!JSObject::deleteProperty(cx, obj, name, &succeeded))
2192         goto error;
2193     if (!succeeded && script->strict) {
2194         obj->reportNotConfigurable(cx, NameToId(name));

So hey, we're testing random memory.  On the site in comment 5 we seem to get lucky in that the memory is reliably 0 (or at least became so in the regression range in comment 7).

Comment 9

[Boris Zbarsky [:bzbarsky]]

So should XPC_WN_Helper_DelProperty just *succeeded = true up front and be done with it?  That certainly fixes the site in comment 5, and I bet it'll fix OpenStreetMap too.

Comment 10

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky [:bz] from comment #9)
> So should XPC_WN_Helper_DelProperty just *succeeded = true up front and be
> done with it?  That certainly fixes the site in comment 5, and I bet it'll
> fix OpenStreetMap too.

Yeah I think so. Apparently the SH hook never got a similar update, and it probably isn't worth doing at this point. r=me on |*succeeded = true|.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Attachment: Make sure to initialize the "succeeded" outparam of XPC_WN_Helper_DelProperty.

Comment 12

[Boris Zbarsky [:bzbarsky]]

https://tbpl.mozilla.org/?tree=Try&rev=a63db13ea426

Comment 13

[Boris Zbarsky [:bzbarsky]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/26cdc56bad7d

Comment 14

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 811993 [details] [diff] [review]
Make sure to initialize the "succeeded" outparam of XPC_WN_Helper_DelProperty.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 858677
User impact if declined: Some sites (e.g. OpenStreetMap) randomly fail to work. 
   Other sites reliably fail to work.
Testing completed (on m-c, etc.): Makes the one reliable testcase we have that
   shows the bug work right.
Risk to taking this patch (and alternatives if risky): I believe this is
   extremely low risk.
String or IDL/UUID changes made by this patch: None

Comment 15

[Jeff Walden [:Waldo]]

I'm told this'll become WebIDL soon, and in there this delete always succeeds, so this seems a totally fine stopgap til WebIDL.  Nice to see this so simply fixed, for branches' sake.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/26cdc56bad7d

Comment 17

[Mariusz Nowak]

Great thanks! Is this scheduled to land in stable FF soon?

Comment 18

[Boris Zbarsky [:bzbarsky]]

I've requested approval to land it on Aurora 26 and Beta 25.  If that's granted (which I expect it will be), this will ship in Firefox 25, on Oct 29 or so.

The only way to get it out there faster is to do a special "chemspill" release.  It's not quite clear to me whether that's worth it; it would shave maybe 2 weeks off the time to getting this out.

If you need an urgent workaround, removeItem should work, or putting the script in non-strict mode for now, right?  :(

Comment 19

[Mariusz Nowak]

Boris, thanks for info. Application is still in dev phase, and I think we can manage with 2 weeks wait.

I just wanted to make sure it'll be addressed for stable channel quite soon. I can hardcode workarounds directly in code, but prefer not to do that.

Comment 20

[Bobby Holley (:bholley)]

(In reply to Mariusz Nowak from comment #19)
> Boris, thanks for info. Application is still in dev phase, and I think we
> can manage with 2 weeks wait.

(Note that it'll be 4 weeks, not 2. 2 is what it would be if we chemspilled).

Comment 21

[Mariusz Nowak]

4 weeks also should be fine, if not, then I'll probably somehow manage that on my side.

I just wonder if it's indeed just few sites that were affected, as it's quite serious when it happens.

Comment 22

[Boris Zbarsky [:bzbarsky]]

Mariusz, the affected sites need to be using delete on a Storage object in a strict-mode script... and then the stack has to end up such that that particular garbage value is 0, not any other value.

So I suspect the number of sites affected is in fact fairly low (if nothing else because of the strict-mode requirement).  And even for some of those it may not happen all the time (e.g. OpenStreetMap), because of the "stack must have a 0" requirement.

That said, I definitely agree this is a serious bug.  That's why I want to backport this to 25 and 26.  The bar for doing a chemspill is a lot higher than that, though: it needs to be either a compat issue that breaks a large fraction of sites (by time visited, not number; so e.g. breaking the single site mail.yahoo.com would count) for a significant fraction of our user base or an actively exploited security problem...  I'm not quite sure that this bug hits that bar, though I'll let the release folks judge that.

Comment 23

[Mariusz Nowak]

Boris, thanks for clarification.

Comment 24

[Alex Keybl [:akeybl]]

Comment on attachment 811993 [details] [diff] [review]
Make sure to initialize the "succeeded" outparam of XPC_WN_Helper_DelProperty.

Given the low risk evaluation here, approving for pre-release branches.

Comment 25

[Boris Zbarsky [:bzbarsky]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/a4b7282df517
https://hg.mozilla.org/releases/mozilla-beta/rev/0c4f0e9b09e9