Closed Bug 1568029 Opened 4 months ago Closed 3 months ago

Slow script dialog from Scratch

Categories

(Core :: JavaScript: GC, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla70
Tracking Status
firefox-esr60 --- unaffected
firefox-esr68 --- unaffected
firefox68 --- unaffected
firefox69 --- fixed
firefox70 --- fixed

People

(Reporter: miketaylr, Assigned: jonco)

References

(Regression, )

Details

(Keywords: regression, Whiteboard: [qf:p2:responsiveness])

Attachments

(1 file)

  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.

Whiteboard: [qf]

NI myself to do a tracelogger investigation.

Flags: needinfo?(dpalmeiro)
Whiteboard: [qf] → [qf:p2:responsiveness]
Component: Untriaged → Performance
Flags: needinfo?(dpalmeiro)
Product: Firefox → Core

Oops, reapply NI so I don't forget.

Flags: needinfo?(dpalmeiro)

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

Component: Performance → JavaScript: GC
Flags: needinfo?(dpalmeiro) → needinfo?(jcoppeard)
Keywords: regression
Regressed by: 1395509

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.

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.

Assignee: nobody → jcoppeard
Flags: needinfo?(jcoppeard)

Wow, this really is all my fault.

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.

Pushed by jcoppeard@mozilla.com:
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?
Status: NEW → RESOLVED
Closed: 3 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla70

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

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:
Flags: needinfo?(jcoppeard)
Attachment #9084086 - Flags: approval-mozilla-beta?

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.

Attachment #9084086 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
See Also: → 1574119
You need to log in before you can comment on or make changes to this bug.