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 (8_0_X) #10894

Closed
wants to merge 6 commits into from
Closed

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

wants to merge 6 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.

This is an 8_0_X backport of #10893

@sgtcoolguy
fix(ios): remove busy loop and change waitForMemoryPanicCleared to no…
ccf8553 
…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 changed the base branch from master to 8_0_X May 15, 2019 17:27
@build build added this to the 8.0.1 milestone May 15, 2019
@build build requested a review from a team May 15, 2019 17:41
@build
Copy link
Contributor

build commented May 15, 2019 •
edited

Messages
📖 👍 Hey!, You deleted more code than you added. That's awesome!
📖

💾 Here's the generated SDK zipfile.
📖

✅ All tests are passing
Nice one! All 3025 tests are passing.

Generated by 🚫 dangerJS against 57730b9

@janvennemann janvennemann changed the title fix(ios): remove busy loop and change waitForMemoryPanicCleared to noop since it's always on main thread fix(ios): remove busy loop and change waitForMemoryPanicCleared to noop since it's always on main thread (8_0_X)) May 15, 2019
@janvennemann janvennemann changed the title fix(ios): remove busy loop and change waitForMemoryPanicCleared to noop since it's always on main thread (8_0_X)) fix(ios): remove busy loop and change waitForMemoryPanicCleared to noop since it's always on main thread (8_0_X) May 15, 2019
@sgtcoolguy sgtcoolguy modified the milestones: 8.0.1, 8.0.2 May 17, 2019
@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

@sgtcoolguy
Copy link
Contributor Author

I pushed another commit to try and address the issue @ssekhri saw - looks like when we get a memory warning, we notify the proxies. If they happen to unregister as a result, I suspect the error he saw occurs as we're iterating over the dictionary containing the proxies.

The "fix" is to iterate over a copy of the keys instead.

As an aside, @janvennemann and @vijaysingh-axway I am a little confused about ow this method works. Looks like we get a memory warning and start notifying proxies, but we stop if the proxy count doesn't change - so if a proxy doesn't do anything about it, why wouldn't we let the other proxies handle it too?

jquick-axway and others added 3 commits May 23, 2019 12:41
@jquick-axway @sgtcoolguy
fix(android)(8_0_X): Improved merge of "tiapp.xml" android manifest s…
b12d883 
…ettings (tidev#10900)

* [TIMOB-26778] Android: Improved merge of "tiapp.xml" file's android manifest settings into Titanium's default manifest settings

- [TIMOB-26778] Fixed bug where overriding an <activity/> in "tiapp.xml" caused most "configChanges" values to be lost.
  * Would cause activity window's UI to disappear if a system config change occurred dynamically.
- [TIMOB-26777] Fixed bug where connecting/disconnecting a physical keyboard to/from device caused UI to disappear.
- [TIMOB-27067] Fixed bug where UI sometimes disappears on Android 9.0 or higher when batter saver turns on/off.
- Added missing <activity/> "configChanges" values:
  * keyboard, layoutDirection, mcc, mnc, navigation, touchscreen, uiMode
- Removed launchMode "singleTask" from "TiMapActivity" and "TiVideoActivity". (Not applicable to child activities.)

* Android: Added new "allActivityConfigChanges" variable to be used by EJS "AndroidManifest.xml" template for [TIMOB-26778]

- Fixed previous [TIMOB-26778] commit to not inject configChanges values to all activities such as those belonging to modules/AARs.
  * Should only be added to Titanium activities that need it. They're now injected via "allActivityConfigChanges" EJS template variable.
- Removed last remnants of "Ti.Map" from build scripts.
- Removed "TiVideoActivity" and "TiCameraActivity" injection code from build script.
  * There is no downside to always having these activities in the "AndroidManifest.xml". So, it's been simplified.

* [TIMOB-27084] Android: Fixed issue where "tiapp.xml" was unable to override "AndroidManifest.xml" settings defined in AAR or "timodule.xml"

- Change manifest merge order. Now merges AAR and "timodule.xml" settings first. "tiapp.xml" settings are merged last.
@jquick-axway @sgtcoolguy
[TIMOB-26895] Android: Fixed issue where APK signing will fail when u…
2023e3b 
…sing MD5 keystore and JDK 8 (tidev#10889)
@sgtcoolguy
fix(ios): iterate over copy of registeredProxies keys not dictionary
3b87853
@vijaysingh-axway
Copy link
Contributor

I pushed another commit to try and address the issue @ssekhri saw - looks like when we get a memory warning, we notify the proxies. If they happen to unregister as a result, I suspect the error he saw occurs as we're iterating over the dictionary containing the proxies.

The "fix" is to iterate over a copy of the keys instead.

As an aside, @janvennemann and @vijaysingh-axway I am a little confused about ow this method works. Looks like we get a memory warning and start notifying proxies, but we stop if the proxy count doesn't change - so if a proxy doesn't do anything about it, why wouldn't we let the other proxies handle it too?

@sgtcoolguy
Inside 'for loop' we are sending memory warning to all registeredProxies. (for loop inside while loop)
If any proxy from registeredProxies get unregistered, while we are sending memory warning, we restart sending memory warning to all proxies inside registeredProxies.
In this way we are sending memory warning to other proxies.
One issue I can see is, if any proxy get unregistered while we are sending memory warning, this will again send memory warning to all registeredProxies though we have already sent warning to some proxies.
Am I missing anything?

@sgtcoolguy
Copy link
Contributor Author

@ssekhri Did the latest commit fix the issue?

@ssekhri
Copy link

ssekhri commented Jun 3, 2019

@sgtcoolguy I will check the latest commit. Unfortunately the ticket is still with In Review status hence didnt get the focus

@ssekhri
Copy link

ssekhri commented Jun 11, 2019

@sgtcoolguy I tried the commit on the PR (8.0.2.v20190530051406) and tried on three different iOS devices.
One device i.e. iPad mini (v9.3.5) on which I reported the issue earlier still shows the crash however did not see any issues when working with iPhone5(v10.3.3) and iPhone 6S(v12.1.4)

@sgtcoolguy
Copy link
Contributor Author

merged manually.

@sgtcoolguy sgtcoolguy closed this Jun 17, 2019
@sgtcoolguy sgtcoolguy deleted the TIMOB-27080-8_0_X branch June 17, 2019 16:38
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 in-qe-testing 🕵 ios no tests
Projects
None yet
Milestone
8.0.2
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@sgtcoolguy @build @ssekhri @vijaysingh-axway @janvennemann @jquick-axway