Closed Bug 1183241 Opened 5 years ago Closed 2 years ago

Freeze, allocation size overflow when using Object.seal() and setting properties

Categories

(Core :: JavaScript Engine, defect)

Unspecified
macOS
defect
Not set

Tracking

()

RESOLVED DUPLICATE of bug 1134253

People

(Reporter: zapperlott, Unassigned)

References

(Depends on 1 open bug)

Details

Attachments

(2 files)

Attached file seal.html
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.
OS: Unspecified → Mac OS X
Component: Untriaged → JavaScript Engine
Product: Firefox → Core
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.
Status: UNCONFIRMED → NEW
Depends on: 633623
Ever confirmed: true
(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?
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.  :-(
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)
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: 2 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1134253
Flags: needinfo?(jwalden+bmo)
Can we add a testcase here?
Flags: needinfo?(andrebargull)
Attached patch bug1183241.patchSplinter Review
Flags: needinfo?(andrebargull)
Attachment #8927945 - Flags: review?(bzbarsky)
Attachment #8927945 - Flags: review?(bzbarsky) → review+
You need to log in before you can comment on or make changes to this bug.