1568029 - Slow script dialog from Scratch

Status: RESOLVED FIXED

(Core :: JavaScript: GC, defect)

Description

[Mike Taylor [:miketaylr]]

1. visit https://llk.github.io/scratch-gui/develop/#225569460
2. let the UI load and try to move around the racecar (top right corner)

Expected: no hangs
Actual: slow script dialog

Profile: https://perfht.ml/2lgm25O , the endless loop is in Blockly.Xml.clearWorkspaceAndLoadFromXml . Chrome doesn't hang in any way.

Comment 1

[Denis Palmeiro [:denispal]]

NI myself to do a tracelogger investigation.

Comment 2

[Denis Palmeiro [:denispal]]

Oops, reapply NI so I don't forget.

Comment 3

[Denis Palmeiro [:denispal]]

This looks like a regression from bug 1395509:

2019-08-06T11:09:01: DEBUG : Found commit message:
Bug 1395509 - Track malloc memory used by ForOfPIC objects r=jandem

This refactors freeing stubs and adds memory tracking. It also adds a back pointer to the JSObject to ForOfPIC::Chain which is slightly annoying, but I think there's only one per global so this shouldn't be too bad.

Differential Revision: https://phabricator.services.mozilla.com/D35348

Comment 4

[Denis Palmeiro [:denispal]]

I rebuilt some of the changes in the regressing bug by hand to confirm and while the bug is right, it looks like the offending change may actually be 340f561967eb from https://phabricator.services.mozilla.com/D35346. This was the first change in the stack that reproduced the problem for me on Windows 10.

Comment 5

[Jon Coppeard (:jonco)]

Super weird that a memory accounting change should cause an infinite loop, but I verified that this problem does seem to be caused by the changeset above.

Comment 6

[Jon Coppeard (:jonco)]

Wow, this really is all my fault.

Comment 7

[Jon Coppeard (:jonco)]

Attachment: Bug 1568029 - Make TypeNewScript::rollbackPartiallyInitializedObjects detect when it's finished traversing the new script initializer list correctly r=tcampbell?

My previous change to the format of the new script initializer list broke this because it relies on finding the sentinel value at the end of the list to know that it didn't break out of the loop early. This means we always call NativeObject::rollbackProperties after this loop when we shouldn't.

The symptom of this is that the workspace property of a Blockly.BlockSvg object becomes undefined causing an infinite loop in code that tries to remove it from that workspace. I verified that the patch fixes this problem.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c03bd9e9ad3e
Make TypeNewScript::rollbackPartiallyInitializedObjects detect when it's finished traversing the new script initializer list correctly r=tcampbell?

Comment 9

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/c03bd9e9ad3e

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Please nominate this for Beta approval when you get a chance.

Comment 11

[Jon Coppeard (:jonco)]

Comment on attachment 9084086 [details]
Bug 1568029 - Make TypeNewScript::rollbackPartiallyInitializedObjects detect when it's finished traversing the new script initializer list correctly r=tcampbell?

Beta/Release Uplift Approval Request

• User impact if declined: Possible incorrect JS execution.
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This change is in a particularly complex area of code. However the fix was simple when the problem was identified. The fix restores the original behaviour before the change that introduced the bug. This has been on central since 9th August without causing problems. I verified that it fixed this issue.
• String changes made/needed:

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9084086 [details]
Bug 1568029 - Make TypeNewScript::rollbackPartiallyInitializedObjects detect when it's finished traversing the new script initializer list correctly r=tcampbell?

Fixes a JS execution regression in Fx69. Approved for 69.0b14.

Comment 13

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-beta/rev/325b59be2036