Closed Bug 906166 Opened 6 years ago Closed 6 years ago

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"

Categories

(Core :: XPConnect, defect)

26 Branch
x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla27
Tracking Status
firefox24 --- affected
firefox25 --- fixed
firefox26 --- fixed

People

(Reporter: john.firebaugh, Assigned: bzbarsky)

References

Details

Attachments

(1 file)

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.
Thanks for reporting this John. 
Could you provide a minimal testcase or an URL to better understand and reproduce this?
Flags: needinfo?(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
Flags: needinfo?(john.firebaugh)
Summary: deleting a localStorage property sometimes results in an error that the property is "non-configurable and can't be deleted" → 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"
I have exactly same problem, and also failed at attempt to produce minimal test case.

It's valid issue.
Component: Untriaged → JavaScript Engine
Product: Firefox → Core
Is there a non-minimal but _reliable_ testcase?
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.
That indeed reproduces reliably, thanks
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).
Blocks: 858677
Status: UNCONFIRMED → NEW
Ever confirmed: true
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.
Flags: needinfo?(jwalden+bmo)
Flags: needinfo?(bobbyholley+bmo)
(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|.
Flags: needinfo?(bobbyholley+bmo)
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
https://hg.mozilla.org/integration/mozilla-inbound/rev/26cdc56bad7d
Flags: in-testsuite?
Target Milestone: --- → mozilla27
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
Attachment #811993 - Flags: approval-mozilla-beta?
Attachment #811993 - Flags: approval-mozilla-aurora?
Component: JavaScript Engine → XPConnect
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.
Flags: needinfo?(jwalden+bmo)
https://hg.mozilla.org/mozilla-central/rev/26cdc56bad7d
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Great thanks! Is this scheduled to land in stable FF soon?
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?  :(
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.
(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).
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.
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.
Boris, thanks for clarification.
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.
Attachment #811993 - Flags: approval-mozilla-beta?
Attachment #811993 - Flags: approval-mozilla-beta+
Attachment #811993 - Flags: approval-mozilla-aurora?
Attachment #811993 - Flags: approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.