Closed Bug 983086 Opened 7 years ago Closed 7 years ago

mailnews/resources/logHelper.js | Error console says [stackFrame mutating the [[Prototype]] of an object will cause your code to run very slowly; instead create the object with the correct initial [[Prototype]] value using Object.create]

Categories

(Thunderbird :: General, defect)

defect
Not set
blocker

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 31.0

People

(Reporter: mkmelin, Assigned: mkmelin)

References

(Blocks 1 open bug)

Details

(Keywords: intermittent-failure)

Attachments

(2 files, 2 obsolete files)

Lots and lots of failures:
TEST-UNEXPECTED-FAIL | ../../../../mailnews/resources/logHelper.js | Error console says [stackFrame mutating the [[Prototype]] of an object will cause your code to run very slowly; instead create the object with the correct initial [[Prototype]] value using Object.create] - See following stack:

This would appear to come from bug 948227.
Attached patch wip (obsolete) — Splinter Review
I think something like this, but i'm not sure if it helped (tests still fail) but i didn't fix all the places so it's possible it's one of the unfixed that still cause the console error msgs.
Note that XYZ.prototype.__proto__ = Foo.prototype is better fixed with
XYZ.prototype = {
  __proto__: Foo.prototype,
  // etc.
};
Attached patch wip2 (obsolete) — Splinter Review
Attachment #8390465 - Attachment is obsolete: true
Looks like try is ok with that - https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=e7a32b98304d

I don't think there's would be a way to achieve the temporary prototype stripping hack in log4moz.js - so i just commented it out.
Assignee: nobody → mkmelin+mozilla
Status: NEW → ASSIGNED
Attachment #8390819 - Flags: review?(standard8)
Comment on attachment 8390819 [details] [diff] [review]
wip2

Found some bugs
Attachment #8390819 - Attachment is obsolete: true
Attachment #8390819 - Flags: review?(standard8)
I'm evalating this bug to blocker since the sheer number of false positive test failures are masking true failures in tests. Until this bug is fixed, I'm closing the tree...
Severity: normal → blocker
This is a temporary patch that keeps the tests from failing simply due to this bug and lets us see where the real failures are in the tree. Considering that uplift is happening very soon, I'm proposing this patch if we can't fix up the tree in time.
Attachment #8391808 - Flags: review?(standard8)
Attached patch proposed fix, v4Splinter Review
This should do it. I'll take the first review i get.
Try was also happy with it (though not the best run) - https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=e216c875c994
Attachment #8391851 - Flags: review?(standard8)
Attachment #8391851 - Flags: review?(Pidgeot18)
Comment on attachment 8391851 [details] [diff] [review]
proposed fix, v4

Review of attachment 8391851 [details] [diff] [review]:
-----------------------------------------------------------------

::: mailnews/base/prefs/content/accountcreation/util.js
@@ -210,5 @@
> -  {
> -    clearInterval(this._id);
> -  }
> -}
> -extend(IntervalAbortable, Abortable);

Any particular reason you're getting rid of the XXX.prototype = { }; calls here?

::: mailnews/db/gloda/modules/log4moz.js
@@ +553,5 @@
>      for each (let [, messageObject] in Iterator(origMessageObjects)) {
>        if (messageObject)
>          if (messageObject._jsonMe) {
>            message.messageObjects.push(messageObject);
> +//          // temporarily strip the prototype to avoid JSONing the impl.

All of the code in this method seems to be focused on making JSON.stringify avoid walking the prototype chain. Reading the documentation on JSON.stringify suggests two other ways this could be done:
1. A custom replacer function. This might make recursive things fun.
2. A Proxy object that only allows access to non-prototype properties.

Either of these would be preferable to the current design, and definitely preferable to commenting out code.
Attachment #8391851 - Flags: review?(Pidgeot18) → feedback+
(In reply to Joshua Cranmer [:jcranmer] from comment #16)
> Comment on attachment 8391851 [details] [diff] [review]
> proposed fix, v4
> 
> Review of attachment 8391851 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mailnews/base/prefs/content/accountcreation/util.js
> @@ -210,5 @@
> > -  {
> > -    clearInterval(this._id);
> > -  }
> > -}
> > -extend(IntervalAbortable, Abortable);
> 
> Any particular reason you're getting rid of the XXX.prototype = { }; calls
> here?

As i understand it you can either use the super __proto__: inside there, or Object.create(<super>.prototype). 
For the very simple cases i used Object.create, but when you have to specify more stuff the __proto__ version is easier to write. OTOH, Object.create looked more explicit to me and so easier for simple cases. 
I don't have a strong opinion about any of it.

> ::: mailnews/db/gloda/modules/log4moz.js
> @@ +553,5 @@
> >      for each (let [, messageObject] in Iterator(origMessageObjects)) {
> >        if (messageObject)
> >          if (messageObject._jsonMe) {
> >            message.messageObjects.push(messageObject);
> > +//          // temporarily strip the prototype to avoid JSONing the impl.
> 
> All of the code in this method seems to be focused on making JSON.stringify
> avoid walking the prototype chain. Reading the documentation on
> JSON.stringify suggests two other ways this could be done:
> 1. A custom replacer function. This might make recursive things fun.
> 2. A Proxy object that only allows access to non-prototype properties.
> 
> Either of these would be preferable to the current design, and definitely
> preferable to commenting out code.

True, I haven't had time to investigate that. With the code commented out we would just print some unnecessary stuff on errors so the harm is manageable.
Comment on attachment 8391851 [details] [diff] [review]
proposed fix, v4

Review of attachment 8391851 [details] [diff] [review]:
-----------------------------------------------------------------

r=Standard8 once you and Joshua have agreed on the way forward with his comments.
Attachment #8391851 - Flags: review?(standard8) → review+
Comment on attachment 8391808 [details] [diff] [review]
Stop the warning from crashing tests

The other one seems to be better & nearly there at the moment.
Attachment #8391808 - Flags: review?(standard8)
(In reply to Magnus Melin from comment #18)
> I don't have a strong opinion about any of it.

I also don't have a terribly strong opinion, it was just that you were replacing XXX.prototype = { }; (particularly non-empty ones) that piqued my interest,.

> True, I haven't had time to investigate that. With the code commented out we
> would just print some unnecessary stuff on errors so the harm is manageable.

I generally don't like commented-out code, but I'm not going to insist that you fix it now. Just file a follow-up bug to handle the thing more properly and note it in the source.
Blocks: 984539
https://hg.mozilla.org/comm-central/rev/efd2920f6586 -> FIXED
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 31.0
You need to log in before you can comment on or make changes to this bug.