Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix(ios): remove busy loop and change waitForMemoryPanicCleared to noop since it's always on main thread #10893

Closed
wants to merge 7 commits into from
Closed

fix(ios): remove busy loop and change waitForMemoryPanicCleared to noop since it's always on main thread #10893

wants to merge 7 commits into from

Conversation

sgtcoolguy
Copy link
Contributor

@sgtcoolguy sgtcoolguy commented May 15, 2019 •
edited

JIRA: https://jira.appcelerator.org/browse/TIMOB-27080

Description:
Was previously used in kroll thread entry points to wait for main thread to handle memory warnings.
Now it will just result in an infinite sleep loop because JS runs on main thread.

All credit goes to @janvennemann for finding this, once he pointed it out to me I was able to do a local SDK build with it as a no-op and confirm the fix on @hansemannn 's Lambus app.

@sgtcoolguy
fix(ios): remove busy loop and change waitForMemoryPanicCleared to no…
1b3c1a0 
…op since it's always on main thread

Was previously used in kroll thread entry points to wait for main thread to handle memory warnings.
Now it will just result in an infinite sleep loop because JS runs on main thread.

Fixes TIMOB-27080
@sgtcoolguy sgtcoolguy mentioned this pull request May 15, 2019
Closed
@janvennemann
Copy link
Contributor

Instead of making it a noop, can't we just remove the method completely? I don't think it's party of any public API so it shouldn't be an issue to remove it now.

@build build added this to the 8.1.0 milestone May 15, 2019
@build build requested a review from a team May 15, 2019 17:32
@build
Copy link
Contributor

build commented May 15, 2019 •
edited

Fails
🚫 Tests have failed, see below for more information.
Messages
📖 👍 Hey!, You deleted more code than you added. That's awesome!
📖

💾 Here's the generated SDK zipfile.
📖 ❌ 3 tests have failed There are 3 tests failing and 464 skipped out of 3613 total tests.

Tests:

ClassnameNameTimeError
android.emulator.Titanium.Android.ServicecreateService-background-normal11.311
Error: timeout of 10000ms exceeded
at Titanium.<anonymous> (/ti-mocha.js:4290:23)
ios.util#inspect() with nested object and infinite depth0.001
Error: expected '{ foo: \'bar\', foobar: 1, func: [ { a: { [Function: a] [arguments]: null, [caller]: null, [length]: 0, [name]: \'a\', [prototype]: a { [constructor]: [Circular] } } }, [length]: 1 ] }' to equal '{ foo: \'bar\', foobar: 1, func: [ { a: { [Function: a] [length]: 0, [name]: \'a\', [prototype]: a { [constructor]: [Circular] } } }, [length]: 1 ] }'
fail@file:///Users/build/Library/Developer/CoreSimulator/Devices/6ECD3876-83E9-4E0B-A372-7F024E62FD30/data/Containers/Bundle/Application/7C74ED9B-9ECB-4135-A0DB-0F7094877FBE/mocha.app/node_modules/should/cjs/should.js:275:23
value@file:///Users/build/Library/Developer/CoreSimulator/Devices/6ECD3876-83E9-4E0B-A372-7F024E62FD30/data/Containers/Bundle/Application/7C74ED9B-9ECB-4135-A0DB-0F7094877FBE/mocha.app/node_modules/should/cjs/should.js:356:23
file:///Users/build/Library/Developer/CoreSimulator/Devices/6ECD3876-83E9-4E0B-A372-7F024E62FD30/data/Containers/Bundle/Application/7C74ED9B-9ECB-4135-A0DB-0F7094877FBE/mocha.app/util.test.js:669:118
run@file:///Users/build/Library/Developer/CoreSimulator/Devices/6ECD3876-83E9-4E0B-A372-7F024E62FD30/data/Containers/Bundle/Application/7C74ED9B-9ECB-4135-A0DB-0F7094877FBE/mocha.app/ti-mocha.js:4372:41
runTest@file:///Users/build/Library/Developer/CoreSimulator/Devices/6ECD3876-83E9-4E0B-A372-7F024E62FD30/data/Containers/Bundle/Application/7C74ED9B-9ECB-4135-A0DB-0F7094877FBE/mocha.app/ti-mocha.js:4759:17
file:///Users/build/Library/Developer/CoreSimulator/Devices/6ECD3876-83E9-4E0B-A372-7F024E62FD30/data/Containers/Bundle/Application/7C74ED9B-9ECB-4135-A0DB-0F7094877FBE/mocha.app/ti-mocha.js:4836:23
next@file:///Users/build/Library/Developer/CoreSimulator/Devices/6ECD3876-83E9-4E0B-A372-7F024E62FD30/data/Containers/Bundle/Application/7C74ED9B-9ECB-4135-A0DB-0F7094877FBE/mocha.app/ti-mocha.js:4684:20
file:///Users/build/Library/Developer/CoreSimulator/Devices/6ECD3876-83E9-4E0B-A372-7F024E62FD30/data/Containers/Bundle/Application/7C74ED9B-9ECB-4135-A0DB-0F7094877FBE/mocha.app/ti-mocha.js:4694:15
next@file:///Users/build/Library/Developer/CoreSimulator/Devices/6ECD3876-83E9-4E0B-A372-7F024E62FD30/data/Containers/Bundle/Application/7C74ED9B-9ECB-4135-A0DB-0F7094877FBE/mocha.app/ti-mocha.js:4632:30
file:///Users/build/Library/Developer/CoreSimulator/Devices/6ECD3876-83E9-4E0B-A372-7F024E62FD30/data/Containers/Bundle/Application/7C74ED9B-9ECB-4135-A0DB-0F7094877FBE/mocha.app/ti-mocha.js:4661:13
timeslice@file:///Users/build/Library/Developer/CoreSimulator/Devices/6ECD3876-83E9-4E0B-A372-7F024E62FD30/data/Containers/Bundle/Application/7C74ED9B-9ECB-4135-A0DB-0F7094877FBE/mocha.app/ti-mocha.js:5760:29
ios.util#inspect() with object0.001
Error: expected '{ foo: \'bar\', foobar: 1, func: { [Function: func] [arguments]: null, [caller]: null, [length]: 0, [name]: \'func\', [prototype]: func { [constructor]: [Circular] } } }' to equal '{ foo: \'bar\', foobar: 1, func: { [Function: func] [length]: 0, [name]: \'func\', [prototype]: func { [constructor]: [Circular] } } }'
fail@file:///Users/build/Library/Developer/CoreSimulator/Devices/6ECD3876-83E9-4E0B-A372-7F024E62FD30/data/Containers/Bundle/Application/7C74ED9B-9ECB-4135-A0DB-0F7094877FBE/mocha.app/node_modules/should/cjs/should.js:275:23
value@file:///Users/build/Library/Developer/CoreSimulator/Devices/6ECD3876-83E9-4E0B-A372-7F024E62FD30/data/Containers/Bundle/Application/7C74ED9B-9ECB-4135-A0DB-0F7094877FBE/mocha.app/node_modules/should/cjs/should.js:356:23
file:///Users/build/Library/Developer/CoreSimulator/Devices/6ECD3876-83E9-4E0B-A372-7F024E62FD30/data/Containers/Bundle/Application/7C74ED9B-9ECB-4135-A0DB-0F7094877FBE/mocha.app/util.test.js:649:17
run@file:///Users/build/Library/Developer/CoreSimulator/Devices/6ECD3876-83E9-4E0B-A372-7F024E62FD30/data/Containers/Bundle/Application/7C74ED9B-9ECB-4135-A0DB-0F7094877FBE/mocha.app/ti-mocha.js:4372:41
runTest@file:///Users/build/Library/Developer/CoreSimulator/Devices/6ECD3876-83E9-4E0B-A372-7F024E62FD30/data/Containers/Bundle/Application/7C74ED9B-9ECB-4135-A0DB-0F7094877FBE/mocha.app/ti-mocha.js:4759:17
file:///Users/build/Library/Developer/CoreSimulator/Devices/6ECD3876-83E9-4E0B-A372-7F024E62FD30/data/Containers/Bundle/Application/7C74ED9B-9ECB-4135-A0DB-0F7094877FBE/mocha.app/ti-mocha.js:4836:23
next@file:///Users/build/Library/Developer/CoreSimulator/Devices/6ECD3876-83E9-4E0B-A372-7F024E62FD30/data/Containers/Bundle/Application/7C74ED9B-9ECB-4135-A0DB-0F7094877FBE/mocha.app/ti-mocha.js:4684:20
file:///Users/build/Library/Developer/CoreSimulator/Devices/6ECD3876-83E9-4E0B-A372-7F024E62FD30/data/Containers/Bundle/Application/7C74ED9B-9ECB-4135-A0DB-0F7094877FBE/mocha.app/ti-mocha.js:4694:15
next@file:///Users/build/Library/Developer/CoreSimulator/Devices/6ECD3876-83E9-4E0B-A372-7F024E62FD30/data/Containers/Bundle/Application/7C74ED9B-9ECB-4135-A0DB-0F7094877FBE/mocha.app/ti-mocha.js:4632:30
file:///Users/build/Library/Developer/CoreSimulator/Devices/6ECD3876-83E9-4E0B-A372-7F024E62FD30/data/Containers/Bundle/Application/7C74ED9B-9ECB-4135-A0DB-0F7094877FBE/mocha.app/ti-mocha.js:4661:13
timeslice@file:///Users/build/Library/Developer/CoreSimulator/Devices/6ECD3876-83E9-4E0B-A372-7F024E62FD30/data/Containers/Bundle/Application/7C74ED9B-9ECB-4135-A0DB-0F7094877FBE/mocha.app/ti-mocha.js:5760:29

Generated by 🚫 dangerJS against c833406

@hansemannn
Copy link
Collaborator

hansemannn commented May 15, 2019 •
edited

Still dead-locking / crashing, but now with many more logs:

[DEBUG] Firing app event: memorywarning
[DEBUG] Firing app event: memorywarning
[DEBUG] Firing app event: memorywarning
[DEBUG] Firing app event: memorywarning
[DEBUG] Firing app event: memorywarning
[DEBUG] Firing app event: memorywarning
[DEBUG] Firing app event: memorywarning
[DEBUG] Firing app event: memorywarning
[DEBUG] Firing app event: memorywarning
[DEBUG] Firing app event: memorywarning
[DEBUG] Firing app event: memorywarning
[DEBUG] Firing app event: memorywarning
[DEBUG] Firing app event: memorywarning
[DEBUG] Firing app event: memorywarning
[DEBUG] Firing app event: memorywarning
[DEBUG] Firing app event: memorywarning
[DEBUG] Firing app event: memorywarning
[DEBUG] Firing app event: memorywarning
[DEBUG] Firing app event: memorywarning

Triggered manually via Debug > Simulate Memory Warning.

EDIT: Discard my comment, I patched an outdated fork of the SDK with this pull. After rebasing from master + applying our 2-3 blocker pulls that we require immediately, this pull works as well!

janvennemann
janvennemann approved these changes May 15, 2019
View reviewed changes
Copy link
Contributor

@janvennemann janvennemann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved! As discussed in Teams we'll keep the function and extern var for now to avoid possible breaking changes.

@ssekhri
Copy link

ssekhri commented May 22, 2019

Still see the issue with the SDK build fetched from Jenkins for this PR. It consistently crashes when scrolling through the trips (had around 15 trips created and it crashes after scrolling through 8-10 trips). Following logs were shown in console during one of the crash
[DEBUG] : Firing app event: memorywarning [DEBUG] : Firing app event: memorywarning [DEBUG] : Firing app event: memorywarning [ERROR] : The application has crashed with an uncaught exception 'NSGenericException'. [ERROR] : Reason: [ERROR] : *** Collection <__NSCFDictionary: 0x1754ea40> was mutated while being enumerated. [ERROR] : Stack trace: [ERROR] : 0 CoreFoundation 0x212ff933 <redacted> + 150 [ERROR] : 1 libobjc.A.dylib 0x20a9ae17 objc_exception_throw + 38 [ERROR] : 2 CoreFoundation 0x212ff3a1 <redacted> + 0 [ERROR] : 3 TitaniumKit 0x01962cc3 -[KrollBridge didReceiveMemoryWarning:] + 196 [ERROR] : 4 CoreFoundation 0x212b1735 <redacted> + 12 [ERROR] : 5 CoreFoundation 0x212b113f <redacted> + 390 [ERROR] : 6 CoreFoundation 0x212b0f1d <redacted> + 40 [ERROR] : 7 CoreFoundation 0x21307c6b <redacted> + 1334 [ERROR] : 8 CoreFoundation 0x21211083 _CFXNotificationPost + 486 [ERROR] : 9 Foundation 0x21a525df <redacted> + 74 [ERROR] : 10 Foundation 0x21a57113 <redacted> + 30 [ERROR] : 11 UIKit 0x25b26813 <redacted> + 166 [ERROR] : 12 libdispatch.dylib 0x20e6d80f <redacted> + 22 [ERROR] : 13 libdispatch.dylib 0x20e85db3 <redacted> + 2030 [ERROR] : 14 libdispatch.dylib 0x20e80d19 <redacted> + 712 [ERROR] : 15 libdispatch.dylib 0x20e7b73f <redacted> + 394 [ERROR] : 16 CoreFoundation 0x212c1b6d <redacted> + 8 [ERROR] : 17 CoreFoundation 0x212c0067 <redacted> + 1574 [ERROR] : 18 CoreFoundation 0x2120f229 CFRunLoopRunSpecific + 520 [ERROR] : 19 CoreFoundation 0x2120f015 CFRunLoopRunInMode + 108 [ERROR] : 20 GraphicsServices 0x227ffac9 GSEventRunModal + 160 [ERROR] : 21 UIKit 0x258e3189 UIApplicationMain + 144 [ERROR] : 22 Lambus 0x000d0871 Lambus + 51313 [ERROR] : 23 libdyld.dylib 0x20eb7873 <redacted> + 2

Env:
SDK: 8.0.2.v20190522114611, 8.1.0.v20190515121638
Device: iPad mini (v9.3.5)
XCode: 10.2.1
tested using Lambus app versioned 1.2.0.9

jawa9000 and others added 4 commits May 23, 2019 12:40
@jawa9000 @sgtcoolguy
Update SystemButton.yml (tidev#10882)
857fe59 
* Update SystemButton.yml

Added system icons image.

https://jira.appcelerator.org/browse/TIDOC-2851

* Create system_icons.png
@ewanharris @sgtcoolguy
build(android): resolve directories relative to file rather than user…
53ca5e3 
…s cwd
@ewanharris @sgtcoolguy
build(js): set corejs version in babel args for rollup
11c9f19 
Also add core-js@2 to our devDependencies so that we dont get burnt if core-js@3 starts being in the top level of the project node_modules
@sgtcoolguy
fix(ios): iterate over copy of registeredProxies keys not dictionary
f7b02c0
@build build added the docs label May 23, 2019
@sgtcoolguy sgtcoolguy modified the milestones: 8.1.0, 8.2.0 Jun 3, 2019
@hansemannn
Copy link
Collaborator

hansemannn commented Jun 4, 2019 •
edited

Any update on the merge here? This is a critical pull request for many production apps. I hope postponing it to 8.2.0 was a mistake.

@sgtcoolguy
Copy link
Contributor Author

@hansemannn It's an artifact of our branching off 8_1_X. We expect this fix to be in 8.0.2, master and 8_1_X.

@ssekhri
Copy link

ssekhri commented Jun 11, 2019

Couldn't check the master build on the lambus app as there are some errors reported in the app upon launch when using the recent builds.
However the PR on 8_0_X branch was tested and the results updated in the PR.

@sgtcoolguy
Copy link
Contributor Author

merged manually

@sgtcoolguy sgtcoolguy closed this Jun 17, 2019
@sgtcoolguy sgtcoolguy deleted the TIMOB-27080 branch June 17, 2019 16:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@janvennemann janvennemann janvennemann approved these changes

Assignees
No one assigned
Labels
bug docs in-qe-testing 🕵 ios no tests
Projects
None yet
Milestone
8.2.0
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@sgtcoolguy @janvennemann @build @hansemannn @ssekhri @jawa9000 @ewanharris