Fix java im sub crash #25505

yunhanw-google · 2023-03-07T01:06:22Z

For active subscription, when we wanna tear down subscription, we need do two things,

ShutDownSubscription from InteractionModelEngine which further call close in readClient,
Remove client pointer in active read client pool in InteractionModelEngine.

When reviewing the Android/Java API,

ShutdownSubscription does nothing via DeviceProxy, it is empty operation there ==> which would noop or crash..
ReportCallback destructor in AndroidCallback.cpp only remove readClient pointer from pool, and does not call shutdown subscription.

When reviewing the darwin code, it seems we also only delete the pointer, it would shutdown subscription when shutdown commissioning.

In order to fix this issue, we need expose the right ShutdownInteraction API from interactionModelEngine to Java/JNI,
1, user needs explicitly call ShutdownSubscription, which further call ReadClient::Close, then destroy client in onDone
2. when fabric is removed, it trigger ReadClient::Close, then destroy client in onDone.
3. when Response timeout or liveness timeout happens, it trigger ReadClient::Close, then destroy client in onDone.

In addition, when onDone is triggered from ShutdownSubscription API in android ui
thread(it acquire lock immediately in ShutdownSubscription ), it further call onDone, but there is another RAII unlock in OnDone which could cause context switch, then eventloop in matter would
acquire the lock in matter thread(https://github.com/project-chip/connectedhomeip/blob/master/src/include/platform/internal/GenericPlatformManagerImpl_POSIX.ipp#L177), complete the work, switch back with
the lock acquired from matter thread, then it delete readClient, and
session, where crash happens when it find the lock's thread id is different
from ui thread since this new lock is from matter thread

github-actions · 2023-03-07T01:25:46Z

PR #25505: Size comparison from a7cc2cf to eb5b85f

Increases (1 build for cc32xx)

platform	target	config	section	`a7cc2cf`	`eb5b85f`	change
cc32xx	lock	CC3235SF_LAUNCHXL	.debug_aranges	87336	87344	8
			.debug_frame	300028	300056	28
			.debug_info	20267068	20267322	254
			.debug_line	2659698	2659803	105
			.debug_loc	2802749	2802977	228
			.debug_ranges	282952	283008	56
			.debug_str	3023892	3023971	79

Full report (1 build for cc32xx)

platform	target	config	section	`a7cc2cf`	`eb5b85f`	change
cc32xx	lock	CC3235SF_LAUNCHXL		0	0	0
			(read only)	644425	644425	0
			(read/write)	203688	203688	0
			.ARM.attributes	44	44	0
			.ARM.exidx	8	8	0
			.bss	197088	197088	0
			.comment	194	194	0
			.data	1480	1480	0
			.debug_abbrev	930235	930235	0
			.debug_aranges	87336	87344	8
			.debug_frame	300028	300056	28
			.debug_info	20267068	20267322	254
			.debug_line	2659698	2659803	105
			.debug_loc	2802749	2802977	228
			.debug_ranges	282952	283008	56
			.debug_str	3023892	3023971	79
			.ramVecs	780	780	0
			.resetVecs	64	64	0
			.rodata	105929	105929	0
			.shstrtab	232	232	0
			.stab	204	204	0
			.stabstr	441	441	0
			.stack	2048	2048	0
			.strtab	378514	378514	0
			.symtab	256624	256624	0
			.text	536372	536372	0

github-actions · 2023-03-07T09:56:46Z

PR #25505: Size comparison from fc0e4a2 to d24046e

Full report (1 build for cc32xx)

platform	target	config	section	`fc0e4a2`	`d24046e`
cc32xx	lock	CC3235SF_LAUNCHXL		0	0
			(read only)	644425	644425
			(read/write)	203688	203688
			.ARM.attributes	44	44
			.ARM.exidx	8	8
			.bss	197088	197088
			.comment	194	194
			.data	1480	1480
			.debug_abbrev	930235	930235
			.debug_aranges	87336	87336
			.debug_frame	300028	300028
			.debug_info	`2026706`	`2026706`
			.debug_line	2659698	2659698
			.debug_loc	2802749	2802749
			.debug_ranges	282952	282952
			.debug_str	3023892	3023892
			.ramVecs	780	780
			.resetVecs	64	64
			.rodata	105929	105929
			.shstrtab	232	232
			.stab	204	204
			.stabstr	441	441
			.stack	2048	2048
			.strtab	378514	378514
			.symtab	256624	256624
			.text	536372	536372

...ler/java/src/com/matter/controller/commands/pairing/PairOnNetworkLongImSubscribeCommand.java

src/controller/java/src/chip/devicecontroller/ChipDeviceController.java

src/controller/java/CHIPDeviceController-JNI.cpp

github-actions · 2023-03-09T06:57:47Z

PR #25505: Size comparison from ecd60f3 to d645d1f

Increases (1 build for cc32xx)

platform	target	config	section	`ecd60f3`	`d645d1f`	change
cc32xx	lock	CC3235SF_LAUNCHXL	.debug_aranges	87344	87352	8
			.debug_frame	300044	300068	24
			.debug_info	20267469	20268488	1019
			.debug_line	2659770	2660062	292
			.debug_loc	`2802853`	2803888	1035
			.debug_str	3024079	3024172	93

Decreases (1 build for cc32xx)

platform	target	config	section	`ecd60f3`	`d645d1f`	change	% change
cc32xx	lock	CC3235SF_LAUNCHXL	.debug_ranges	282960	282880	-80	-0.0

Full report (4 builds for cc32xx, mbed, qpg)

platform	target	config	section	`ecd60f3`	`d645d1f`	change	% change
cc32xx	lock	CC3235SF_LAUNCHXL		0	0	0	0.0
			(read only)	644425	644425	0	0.0
			(read/write)	203688	203688	0	0.0
			.ARM.attributes	44	44	0	0.0
			.ARM.exidx	8	8	0	0.0
			.bss	197088	197088	0	0.0
			.comment	194	194	0	0.0
			.data	1480	1480	0	0.0
			.debug_abbrev	930235	930235	0	0.0
			.debug_aranges	87344	87352	8	0.0
			.debug_frame	300044	300068	24	0.0
			.debug_info	20267469	20268488	1019	0.0
			.debug_line	2659770	2660062	292	0.0
			.debug_loc	`2802853`	2803888	1035	0.0
			.debug_ranges	282960	282880	-80	-0.0
			.debug_str	3024079	3024172	93	0.0
			.ramVecs	780	780	0	0.0
			.resetVecs	64	64	0	0.0
			.rodata	105929	105929	0	0.0
			.shstrtab	232	232	0	0.0
			.stab	204	204	0	0.0
			.stabstr	441	441	0	0.0
			.stack	2048	2048	0	0.0
			.strtab	378571	378571	0	0.0
			.symtab	256624	256624	0	0.0
			.text	536372	536372	0	0.0
mbed	lock-app	CY8CPROTO_062_4343W+release	(read only)	6224	6224	0	0.0
			(read/write)	2467664	2467664	0	0.0
			.bss	215804	215804	0	0.0
			.data	5880	5880	0	0.0
			.text	1430308	1430308	0	0.0
qpg	lighting-app	qpg6105+debug	(read/write)	`1153116`	`1153116`	0	0.0
			.bss	99916	99916	0	0.0
			.data	852	852	0	0.0
			.text	600212	600212	0	0.0
	lock-app	qpg6105+debug	(read/write)	1119524	1119524	0	0.0
			.bss	96308	96308	0	0.0
			.data	864	864	0	0.0
			.text	566624	566624	0	0.0

src/app/InteractionModelEngine.h

src/controller/java/src/chip/devicecontroller/ChipDeviceController.java

...id/CHIPTool/app/src/main/java/com/google/chip/chiptool/clusterclient/SensorClientFragment.kt

github-actions · 2023-03-10T10:50:15Z

PR #25505: Size comparison from bdee2ef to 0f3029d

Increases (1 build for cc32xx)

platform	target	config	section	`bdee2ef`	`0f3029d`	change	% change
cc32xx	lock	CC3235SF_LAUNCHXL	.debug_info	20275029	`2027507`	47	0.0
			.debug_loc	`2804674`	2804731	57	0.0

Decreases (1 build for cc32xx)

platform	target	config	section	`bdee2ef`	`0f3029d`	change	% change
cc32xx	lock	CC3235SF_LAUNCHXL	.debug_frame	300236	300224	-12	-0.0
			.debug_line	2660914	2660781	-133	-0.0
			.debug_ranges	283248	283136	-112	-0.0

Full report (1 build for cc32xx)

platform	target	config	section	`bdee2ef`	`0f3029d`	change	% change
cc32xx	lock	CC3235SF_LAUNCHXL		0	0	0	0.0
			(read only)	644945	644945	0	0.0
			(read/write)	203848	203848	0	0.0
			.ARM.attributes	44	44	0	0.0
			.ARM.exidx	8	8	0	0.0
			.bss	197248	197248	0	0.0
			.comment	194	194	0	0.0
			.data	1480	1480	0	0.0
			.debug_abbrev	930293	930293	0	0.0
			.debug_aranges	87376	87376	0	0.0
			.debug_frame	300236	300224	-12	-0.0
			.debug_info	20275029	`2027507`	47	0.0
			.debug_line	2660914	2660781	-133	-0.0
			.debug_loc	`2804674`	2804731	57	0.0
			.debug_ranges	283248	283136	-112	-0.0
			.debug_str	3026077	3026077	0	0.0
			.ramVecs	780	780	0	0.0
			.resetVecs	64	64	0	0.0
			.rodata	105921	105921	0	0.0
			.shstrtab	232	232	0	0.0
			.stab	204	204	0	0.0
			.stabstr	441	441	0	0.0
			.stack	2048	2048	0	0.0
			.strtab	379152	379152	0	0.0
			.symtab	256848	256848	0	0.0
			.text	536904	536904	0	0.0

When onDone is triggered from ShutdownSubscription API in android ui thread, RAII unlock in OnDone could cause context switch, then eventloop in matter would acquire the lock in matter thread, complete the work, switch back with the lock acquired from matter thread, then it delete readClient, and session, where it crash when it find the lock's thread id is different from ui thread. The fix is to move the stackUnlock after readClient delete operation. enable lock error detection

github-actions · 2023-03-10T11:10:16Z

PR #25505: Size comparison from bdee2ef to 65f801f

Increases (1 build for cc32xx)

platform	target	config	section	`bdee2ef`	`65f801f`	change	% change
cc32xx	lock	CC3235SF_LAUNCHXL	.debug_info	20275029	20275075	46	0.0
			.debug_loc	`2804674`	2804731	57	0.0

Decreases (1 build for cc32xx)

platform	target	config	section	`bdee2ef`	`65f801f`	change	% change
cc32xx	lock	CC3235SF_LAUNCHXL	.debug_frame	300236	300224	-12	-0.0
			.debug_line	2660914	2660781	-133	-0.0
			.debug_ranges	283248	283136	-112	-0.0

Full report (1 build for cc32xx)

platform	target	config	section	`bdee2ef`	`65f801f`	change	% change
cc32xx	lock	CC3235SF_LAUNCHXL		0	0	0	0.0
			(read only)	644945	644945	0	0.0
			(read/write)	203848	203848	0	0.0
			.ARM.attributes	44	44	0	0.0
			.ARM.exidx	8	8	0	0.0
			.bss	197248	197248	0	0.0
			.comment	194	194	0	0.0
			.data	1480	1480	0	0.0
			.debug_abbrev	930293	930293	0	0.0
			.debug_aranges	87376	87376	0	0.0
			.debug_frame	300236	300224	-12	-0.0
			.debug_info	20275029	20275075	46	0.0
			.debug_line	2660914	2660781	-133	-0.0
			.debug_loc	`2804674`	2804731	57	0.0
			.debug_ranges	283248	283136	-112	-0.0
			.debug_str	3026077	3026077	0	0.0
			.ramVecs	780	780	0	0.0
			.resetVecs	64	64	0	0.0
			.rodata	105921	105921	0	0.0
			.shstrtab	232	232	0	0.0
			.stab	204	204	0	0.0
			.stabstr	441	441	0	0.0
			.stack	2048	2048	0	0.0
			.strtab	379152	379152	0	0.0
			.symtab	256848	256848	0	0.0
			.text	536904	536904	0	0.0

- Pull in latest Matter SDK (SHA: 3c6bed3bb2) which includes required fixes for subscriptions (project-chip/connectedhomeip#25505, project-chip/connectedhomeip#25623, project-chip/connectedhomeip#25625) - Strip the .so's prior to copying them to jniLibs. libCHIPController.so now has a reasonable size (went from 352MB to 26MB). - Made SubscriptionHelper functions that interact with a device as "suspend" functions so that the coroutine scope is established by the caller. - Removed flag UNSUBSCRIBE_ENABLED since shutdownSubscriptions has now been fixed in the Matter SDK.

* Fix java im sub crash * address comments When onDone is triggered from ShutdownSubscription API in android ui thread, RAII unlock in OnDone could cause context switch, then eventloop in matter would acquire the lock in matter thread, complete the work, switch back with the lock acquired from matter thread, then it delete readClient, and session, where it crash when it find the lock's thread id is different from ui thread. The fix is to move the stackUnlock after readClient delete operation. enable lock error detection

yunhanw-google marked this pull request as draft March 7, 2023 01:06

github-actions bot added app controller labels Mar 7, 2023

yunhanw-google force-pushed the feature/fix_java_im_sub branch from eb5b85f to d24046e Compare March 7, 2023 09:40

github-actions bot added examples lib support and removed app labels Mar 7, 2023

yunhanw-google force-pushed the feature/fix_java_im_sub branch from d24046e to 293d016 Compare March 7, 2023 18:52

yunhanw-google marked this pull request as ready for review March 7, 2023 22:41

g-coppock suggested changes Mar 7, 2023

View reviewed changes

This was referenced Mar 8, 2023

GHSAFM crashed with Fatal signal 6 (SIGABRT) google-home/sample-apps-for-matter-android#91

Closed

[native] native crash google-home/sample-apps-for-matter-android#87

Closed

joonhaengHeo reviewed Mar 8, 2023

View reviewed changes

src/controller/java/CHIPDeviceController-JNI.cpp Outdated Show resolved Hide resolved

yunhanw-google force-pushed the feature/fix_java_im_sub branch from ca631a7 to c207e29 Compare March 8, 2023 00:40

github-actions bot added app darwin tools labels Mar 8, 2023

yunhanw-google force-pushed the feature/fix_java_im_sub branch 2 times, most recently from 659a6cd to 2a15f26 Compare March 8, 2023 01:43

pullapprove bot requested review from amitnj, andy31415, anush-apple, arkq, Byungjoo-Lee, bzbarsky-apple, carol-apple and chrisdecenzo March 8, 2023 01:44

yunhanw-google force-pushed the feature/fix_java_im_sub branch 3 times, most recently from 1311c9a to d645d1f Compare March 9, 2023 06:32

bzbarsky-apple requested changes Mar 9, 2023

View reviewed changes

src/app/InteractionModelEngine.h Outdated Show resolved Hide resolved

pullapprove bot added review - pending and removed review - approved labels Mar 9, 2023

g-coppock approved these changes Mar 9, 2023

View reviewed changes

yunhanw-google force-pushed the feature/fix_java_im_sub branch from d645d1f to 174ec11 Compare March 10, 2023 10:36

Fix java im sub crash

698ebaa

yunhanw-google force-pushed the feature/fix_java_im_sub branch from 174ec11 to c46c94b Compare March 10, 2023 10:36

yunhanw-google requested review from bzbarsky-apple and removed request for pjzander-signify March 10, 2023 10:38

yunhanw-google force-pushed the feature/fix_java_im_sub branch from c46c94b to 0f3029d Compare March 10, 2023 10:39

yunhanw-google force-pushed the feature/fix_java_im_sub branch from 0f3029d to 65f801f Compare March 10, 2023 10:53

yunhanw-google enabled auto-merge (squash) March 10, 2023 11:05

bzbarsky-apple approved these changes Mar 10, 2023

View reviewed changes

pullapprove bot added review - approved and removed review - pending labels Mar 10, 2023

yunhanw-google merged commit a8b42c1 into project-chip:master Mar 10, 2023

yunhanw-google mentioned this pull request Mar 12, 2023

Fix kotlin test CI and android TV cast example CI #25644

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix java im sub crash #25505

Fix java im sub crash #25505

yunhanw-google commented Mar 7, 2023 •

edited

Loading

github-actions bot commented Mar 7, 2023

github-actions bot commented Mar 7, 2023

github-actions bot commented Mar 9, 2023

github-actions bot commented Mar 10, 2023

github-actions bot commented Mar 10, 2023

Fix java im sub crash #25505

Fix java im sub crash #25505

Conversation

yunhanw-google commented Mar 7, 2023 • edited Loading

github-actions bot commented Mar 7, 2023

github-actions bot commented Mar 7, 2023

github-actions bot commented Mar 9, 2023

github-actions bot commented Mar 10, 2023

github-actions bot commented Mar 10, 2023

yunhanw-google commented Mar 7, 2023 •

edited

Loading