Closed
Bug 983086
Opened 10 years ago
Closed 10 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)
Thunderbird
General
Tracking
(Not tracked)
RESOLVED
FIXED
Thunderbird 31.0
People
(Reporter: mkmelin, Assigned: mkmelin)
References
Details
(Keywords: intermittent-failure)
Attachments
(2 files, 2 obsolete files)
1.93 KB,
patch
|
Details | Diff | Splinter Review | |
21.29 KB,
patch
|
standard8
:
review+
jcranmer
:
feedback+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•10 years ago
|
||
https://tbpl.mozilla.org/php/getParsedLog.php?id=36028024&tree=Thunderbird-Trunk&full=1
Comment hidden (Legacy TBPL/Treeherder Robot) |
Assignee | ||
Comment 3•10 years ago
|
||
I guess these should be fixed: http://mxr.mozilla.org/comm-central/search?string=.__proto__+%3D&find=mail&findi=&filter=^[^\0]*%24&hitlimit=&tree=comm-central
Assignee | ||
Comment 4•10 years ago
|
||
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.
Comment 5•10 years ago
|
||
Note that XYZ.prototype.__proto__ = Foo.prototype is better fixed with XYZ.prototype = { __proto__: Foo.prototype, // etc. };
Comment hidden (Legacy TBPL/Treeherder Robot) |
Assignee | ||
Comment 7•10 years ago
|
||
Attachment #8390465 -
Attachment is obsolete: true
Assignee | ||
Comment 8•10 years ago
|
||
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
Assignee | ||
Updated•10 years ago
|
Attachment #8390819 -
Flags: review?(standard8)
Comment hidden (Legacy TBPL/Treeherder Robot) |
Assignee | ||
Comment 10•10 years ago
|
||
Comment on attachment 8390819 [details] [diff] [review] wip2 Found some bugs
Attachment #8390819 -
Attachment is obsolete: true
Attachment #8390819 -
Flags: review?(standard8)
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment 12•10 years ago
|
||
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
Comment 13•10 years ago
|
||
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)
Comment 14•10 years ago
|
||
Try push for the above patch: <https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=87407a825bbf>.
Assignee | ||
Comment 15•10 years ago
|
||
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 16•10 years ago
|
||
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+
Comment hidden (Legacy TBPL/Treeherder Robot) |
Assignee | ||
Comment 18•10 years ago
|
||
(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 19•10 years ago
|
||
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 20•10 years ago
|
||
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)
Comment 21•10 years ago
|
||
(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.
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Assignee | ||
Comment 24•10 years ago
|
||
https://hg.mozilla.org/comm-central/rev/efd2920f6586 -> FIXED
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 31.0
Comment hidden (Legacy TBPL/Treeherder Robot) |
You need to log in
before you can comment on or make changes to this bug.
Description
•