Closed
Bug 1183241
Opened 9 years ago
Closed 7 years ago
Freeze, allocation size overflow when using Object.seal() and setting properties
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
DUPLICATE
of bug 1134253
People
(Reporter: zapperlott, Unassigned)
References
(Depends on 1 open bug)
Details
Attachments
(2 files)
(deleted),
text/html
|
Details | |
(deleted),
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_10_4) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/43.0.2357.132 Safari/537.36
Steps to reproduce:
In short:
var obj = { /* complex object */ };
Object.seal(obj);
obj.newProp = 1;
Long story:
In our JavaScript application, Firefox freezes reproducibly on one page. I was able to track it down to a property creation on a sealed object. Since the code is not running in Strict Mode, no TypeError is raised. (That’s a bug in our code, but all other browsers just ignore the property creation and do not freeze.)
In our JS app, Firefox first shows “long running script” warning multiple times. If I choose to debug, the debugger stops in the line that performs
sealedObject.newProperty = someValue
If I choose “continue”, Firefox’ memory consumption grows and grows, then after some time the JS engine stops the script with an “allocation size overflow” error on the console.
I was able to isolate the problem, see attached file.
Actual results:
Executing “o.newProp = i + 1;” takes roughly 1s on my machine.
The test also shows how the complexity of the sealed object (see init phase) affects the property setting performance (set phase).
You can play around with “l” to produce a freeze.
Expected results:
The test case should run through in a reasonable amount of time.
I know that Object.seal() impairs the performance of operations on the sealed object significantly, but FF should not freeze easily when setting properties.
Reporter | ||
Updated•9 years ago
|
OS: Unspecified → Mac OS X
Updated•9 years ago
|
Component: Untriaged → JavaScript Engine
Product: Firefox → Core
Comment 1•9 years ago
|
||
This doesn't have anything to do with Object.seal, property-setting, or any of that. The problem is that in your testcase, the strict mode error we throw has the error message "{0} is not extensible", where {0} is replaced with a stringified representation of the object on which a property is set. This stringified representation invokes a toSource function on the object in question. In your case, that function then invokes toSource on the subcomponents of the object, and so on, nested. That's 600000 function calls, producing a similar number of strings, executing a whole bunch of code.
So really, when you consider how long it takes the code to run (I can run it just fine in a shell, if I give it long enough), this is just a different flavor of bug 633623.
Reporter | ||
Comment 2•9 years ago
|
||
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #1)
> in your testcase, the strict mode error we throw has the error message
> "{0} is not extensible", where {0} is replaced with a stringified representation
> of the object on which a property is set.
Okay, thanks. But the code in question isn’t running in Strict Mode. No exception is being thrown. Is the error message still being generated, just not thrown / shown on the console?
Comment 3•9 years ago
|
||
Yes, I believe so, unfortunately. Our error reporting mechanism requires providing the strings to interpolate into the ultimate message, regardless whether an error is thrown, a warning is reported to the console, or nothing at all occurs. :-(
Comment 4•9 years ago
|
||
We should strongly consider not outputting the full toSource of the object here, precisely because of issues like this. Cap the depth or the overall string length or _something_.
Flags: needinfo?(jwalden+bmo)
Comment 5•7 years ago
|
||
Apparently unintentionally fixed by bug 1134253, which changed the error message from JSMSG_OBJECT_NOT_EXTENSIBLE to JSMSG_CANT_DEFINE_PROP_OBJECT_NOT_EXTENSIBLE and only JSMSG_OBJECT_NOT_EXTENSIBLE decompiles the object [1].
[1] https://searchfox.org/mozilla-central/rev/30ead7d1ae5bf95b8bc0fd67b950cd46ca05e32c/js/src/jsapi.cpp#164-168
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → DUPLICATE
Updated•7 years ago
|
Flags: needinfo?(jwalden+bmo)
Comment 7•7 years ago
|
||
Flags: needinfo?(andrebargull)
Attachment #8927945 -
Flags: review?(bzbarsky)
Updated•7 years ago
|
Attachment #8927945 -
Flags: review?(bzbarsky) → review+
Updated•7 years ago
|
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/440e896d762a
Add a regression test case. r=bz
Keywords: checkin-needed
Comment 9•7 years ago
|
||
bugherder |
You need to log in
before you can comment on or make changes to this bug.
Description
•