1238763 - crash in java.lang.IllegalThreadStateException: Expected anything but 1 ("main"), but running there. at org.mozilla.gecko.util.ThreadUtils.assertOnThreadComparison(ThreadUtils.java)

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Kartikaya Gupta (email:[e-mail])]

This bug was filed from the Socorro interface and is 
report bp-aa8465e3-0fac-47f6-8d3f-d2a672160111.
=============================================================

STR: add a bookmark, then click on the options in the toast and add the bookmark to home screen.

Comment 1

[Richard Newman [:rnewman]]

This is why we add assertions and make them fail before release :)

        at org.mozilla.gecko.util.ThreadUtils.assertNotOnUiThread(ThreadUtils.java:127)
	at org.mozilla.gecko.db.LocalURLMetadata.getForURLs(LocalURLMetadata.java:149)
	at org.mozilla.gecko.GeckoAppShell.createShortcut(GeckoAppShell.java:820)
	at org.mozilla.gecko.BrowserApp$20.onPromptFinished(BrowserApp.java:1119)

Comment 2

[Andrzej Hunt :ahunt]

So it turns out our prompt callbacks are run on the UI thread - I think it's best to run all callbacks in an AsyncTask (patch will be attached shortly). The alternative would be to use an AsyncTask just for our createShortcut call, but that feels more awkward to me...

Comment 3

[Andrzej Hunt :ahunt]

Attachment: MozReview Request: Bug 1238763 - Pre: remove unneeded imports r=liuche

Review commit: https://reviewboard.mozilla.org/r/30435/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/30435/

Comment 4

[Mark Finkle (:mfinkle) (use needinfo?)]

https://reviewboard.mozilla.org/r/30435/#review27207

::: mobile/android/base/java/org/mozilla/gecko/prompts/Prompt.java:506
(Diff revision 1)
> +            new AsyncTask<Void, Void, Void>() {

You might just want to use our:
ThreadUtils.postToBackgroundThread(new Runnable() {...});

Comment 5

[Andrzej Hunt :ahunt]

Comment on attachment 8706671 [details]
MozReview Request: Bug 1238763 - Pre: remove unneeded imports r=liuche

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/30435/diff/1-2/

Comment 6

[Andrzej Hunt :ahunt]

Attachment: MozReview Request: Bug 1238763 - Part 1: Don't run onPromptFinished on the UI thread. r=liuche

Review commit: https://reviewboard.mozilla.org/r/30569/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/30569/

Comment 7

[Chenxia Liu [:liuche]]

Comment on attachment 8706671 [details]
MozReview Request: Bug 1238763 - Pre: remove unneeded imports r=liuche

https://reviewboard.mozilla.org/r/30435/#review27377

Comment 8

[Chenxia Liu [:liuche]]

Comment on attachment 8707071 [details]
MozReview Request: Bug 1238763 - Part 1: Don't run onPromptFinished on the UI thread. r=liuche

https://reviewboard.mozilla.org/r/30569/#review27379

lgtm! You probably don't need the Part 1, unless there's more.

Comment 9

[Andrzej Hunt :ahunt]

https://hg.mozilla.org/integration/fx-team/rev/3e0daa666d7aab1d8fdaf4e9dad1a25fd3356deb
Bug 1238763 - Pre: remove unneeded imports r=liuche

https://hg.mozilla.org/integration/fx-team/rev/431f531e8bff4d1e5a0b315f4aa7ac42f307ef21
Bug 1238763 - Don't run onPromptFinished on the UI thread. r=liuche

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3e0daa666d7a
https://hg.mozilla.org/mozilla-central/rev/431f531e8bff

Comment 11

[Kartikaya Gupta (email:[e-mail])]

This doesn't crash now but I also don't get any confirmation after clicking on the "add to home screen" option. It added the icon but didn't provide any feedback inside fennec itself. Don't know if that's intentional or not.

Comment 12

[Andrzej Hunt :ahunt]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #11)
> This doesn't crash now but I also don't get any confirmation after clicking
> on the "add to home screen" option. It added the icon but didn't provide any
> feedback inside fennec itself. Don't know if that's intentional or not.

That's expected, and something we want to fix in Bug 1240560!

Comment 13

[Andrzej Hunt :ahunt]

I'll be reverting this fix (and providing a different fix) in Bug 1238763 - for consistency with Android conventions, and for simplicity, the callback should probably be on the UI thread, and the callee can then be responsible for pushing code to the background thread as needed.

(This will avoid transferring from UI thread to background thread and back to the UI thread in those cases where the callback needs to do UI actions.)