Closed Bug 995803 Opened 6 years ago Closed 6 years ago

crash in java.lang.IllegalStateException: Already registered Webapps:Preinstall at org.mozilla.gecko.EventDispatcher.registerListener(EventDispatcher.java)

Categories

(Firefox for Android :: Web Apps (PWAs), defect, P1, blocker)

31 Branch
All
Android
defect

Tracking

()

VERIFIED FIXED
Firefox 32
Tracking Status
firefox29 --- unaffected
firefox30 --- unaffected
firefox31 --- verified
firefox32 --- verified

People

(Reporter: aaronmt, Assigned: jhugman)

References

Details

(Keywords: crash, regression, reproducible, Whiteboard: [WebRuntime])

Crash Data

Attachments

(2 files, 1 obsolete file)

This bug was filed from the Socorro interface and is 
report bp-9fd0d253-3aa2-4652-ab90-086372140408.
=============================================================

java.lang.IllegalStateException: Already registered Webapps:Preinstall
	at org.mozilla.gecko.EventDispatcher.registerListener(EventDispatcher.java:57)
	at org.mozilla.gecko.EventDispatcher.registerEventListener(EventDispatcher.java:132)
	at org.mozilla.gecko.webapp.EventListener.registerEventListener(EventListener.java:57)
	at org.mozilla.gecko.webapp.EventListener.registerEvents(EventListener.java:65)
	at org.mozilla.gecko.GeckoApp.onWindowFocusChanged(GeckoApp.java:2028)
	at com.android.internal.policy.impl.PhoneWindow$DecorView.onWindowFocusChanged(PhoneWindow.java:2462)
	at android.view.View.dispatchWindowFocusChanged(View.java:7578)
	at android.view.ViewGroup.dispatchWindowFocusChanged(ViewGroup.java:960)
	at android.view.ViewRootImpl$ViewRootHandler.handleMessage(ViewRootImpl.java:3115)
	at android.os.Handler.dispatchMessage(Handler.java:99)
	at android.os.Looper.loop(Looper.java:137)
	at android.app.ActivityThread.main(ActivityThread.java:5103)
	at java.lang.reflect.Method.invokeNative(Native Method)
	at java.lang.reflect.Method.invoke(Method.java:525)
	at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:737)
	at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:553)
	at dalvik.system.NativeStart.main(Native Method)
Priority: -- → P1
Whiteboard: [WebRuntime]
Reproducible (Nightly 04/15, Nexus 5 (Android 4.4.2).

Steps:

   i) Installed Twitter
   ii) Launched Twitter from the 'PackageManager' installation complete dialog
   iii) Re-launched Twitter again from my application listing on my device
Keywords: reproducible
Keywords: regression
About ~15 crash reports as of today (04/21)
Severity: critical → blocker
Assignee: nobody → jhugman
I can reliably reproduce this, however it only causes an exception in the logs, rather than crashing the app or Fennec.
My STR (which unlike Comment 1, don't require you to reinstall Twitter each time):

 0. (Have twitter Open Web App installed)
 1. Launch Twitter Open Web App (e.g. via launcher on home screen)
 2. In android Settings | Apps | Downloaded | Twitter, hit "Force Stop" button.
 3. Re-launch Twitter Open Web App.

ACTUAL RESULTS: Crash report dialog appears, on startup.
bp-ad2e03f4-fd4e-4cdb-a7aa-5a9b92140424
bp-02d06f8f-14a5-4c76-a2cd-d0baa2140424
This happened and I got lolcat during it:
http://pastebin.mozilla.org/4936485
:ozten - what device/OS version are you using? And is fennec hard crashing for you?
Flags: needinfo?(ozten.bugs)
Duplicate of this bug: 1008237
The issue is that we are registering this event in InstallHelper.java and also in EventListener.java. It seems that this error is preventing the 'Webapps:AutoInstall' message from being broadcast from within the install function in InstallHelper. Commenting out the 'registerGeckoListener();' line in the install function in InstallHelper allows this event to be broadcast.
Attachment #8421065 - Flags: review?(wjohnson)
Attachment #8421065 - Flags: review?(mark.finkle)
Attachment #8421065 - Flags: feedback?(myk)
Attachment #8421065 - Flags: feedback?(mhaigh)
Comment on attachment 8421065 [details] [diff] [review]
Fixed the stacktrace, although unable to recreate the crash.

Review of attachment 8421065 [details] [diff] [review]:
-----------------------------------------------------------------

I don't have feedback to provide about the core of the patch.  It looks reasonable, and it resembles the way other GeckoEventListeners work, but I'm not familiar enough with that class to know why our current approach is buggy.  So it'd be useful to get some context, i.e. an explanation of why we implemented it that way originally and why this change is correct.

::: mobile/android/base/GeckoApp.java
@@ +396,4 @@
>                  onPreparePanel(featureId, mMenuPanel, mMenu);
>              }
>  
> +            return mMenuPanel;

Avoid these unrelated whitespace changes, which makes the patch harder to review and pollutes history/blame.

@@ +2253,4 @@
>          }
>      }
>  
> +    @Override

Avoid these unrelated (and unnecessary) changes.

::: mobile/android/base/webapp/WebappImpl.java
@@ +260,4 @@
>                      // Don't show the titlebar for about:blank, which we load
>                      // into the initial tab we create while waiting for the app
>                      // to load.
> +                    if (urlString == null || urlString.equals("about:blank")) {

Why remove the titlebar when urlString is null?  At the very least, the comment here needs updating to explain why this now removes the titlebar under that additional circumstance.
Attachment #8421065 - Flags: feedback?(myk)
I think Myk covered everything I wanted to say.
Attachment #8421065 - Flags: feedback?(mhaigh)
Grr. Whitespaces/@Overrides. I thought I'd removed them. 

Approach has been to register one EventListener per GeckoApp. Previously, this was using a static EventListener, which was unclear as to which GeckApp the EventListener was communicating with, and caused this problem. The problem arose when multiple GeckoApp instances were being initialized, but only one EventListener was being used.

The possible solutions to this were:

 1. move the initialization check in to EventListener, so one EventListener could be used for multiple GeckoApps. This is the smallest change to make.
 2. Make EventListener non-static, and initialize it at the same time as GeckoApp.
 3. Investigate why two GeckoApp instances are being initialized. The STR would indicate that changing launch flags in Dispatcher may fix the problem, but I couldn't find a combination that would work.

My suspicion has been that using 3. would fix the bug once and for all, but in the absence of being able to reproduce, fixing the stack trace moves this problem forward.

>                      // Don't show the titlebar for about:blank, which we load
>                      // into the initial tab we create while waiting for the app
>                      // to load.
> +                    if (urlString == null || urlString.equals("about:blank")) {

urlString != null is assumed fairly soon after this line, but null urlString isn't otherwise being dealt with.
Comment on attachment 8421065 [details] [diff] [review]
Fixed the stacktrace, although unable to recreate the crash.

Let's get this in Nightly and see if it fixes the issues, since other can reproduce it.

r+ with Myk and Martyn feedback addressed
Attachment #8421065 - Flags: review?(mark.finkle) → review+
Attachment #8421065 - Attachment is obsolete: true
Attachment #8421065 - Flags: review?(wjohnson)
Comment on attachment 8424880 [details] [diff] [review]
Addressed nits from mfinkle's r+

jhugman: it'd be better to leave the "review" flag off this patch, so the sheriff who commits it doesn't think it's r=jhugman.  Thus I'm removing the flag and will also un-obsolete your previous patch, so it shows up in the list of attachments with mfinkle's review, and the sheriff can see that mfinkle is the peer who gave the review.

Also, note that your username in this patch is "jhugman <jhugman@jhugman-10147.local>", whereas it's been "James Hugman <jhugman@mozilla.com>" in the past.  Presumably the latter is the correct value, in which case you might want to upload a new patch that corrects it!
Attachment #8424880 - Flags: review+
Attachment #8421065 - Attachment is obsolete: false
Comment on attachment 8421065 [details] [diff] [review]
Fixed the stacktrace, although unable to recreate the crash.

Please leave obsolete patches marked as such. :)
Attachment #8421065 - Attachment is obsolete: true
https://hg.mozilla.org/integration/fx-team/rev/67ec94062362
Keywords: checkin-needed
Whiteboard: [WebRuntime] → [WebRuntime][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/67ec94062362
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [WebRuntime][fixed-in-fx-team] → [WebRuntime]
Target Milestone: --- → Firefox 32
James: is this problem present on Aurora and/or Beta as well; and if so, can you request uplift for it?  It's a pretty bad problem, so it's worth uplifting it if it affects those branches.
Flags: needinfo?(jhugman)
Comment on attachment 8424880 [details] [diff] [review]
Addressed nits from mfinkle's r+

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: 
Under fairly common webapp circumstances, Gecko actualy crashes.
Testing completed (on m-c, etc.): 
On m-c.
Risk to taking this patch (and alternatives if risky):
None.
String or IDL/UUID changes made by this patch:
None.
Attachment #8424880 - Flags: approval-mozilla-aurora?
Flags: needinfo?(jhugman)
Attachment #8424880 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Needs a branch patch for Aurora uplift.
Flags: needinfo?(jhugman)
Comment on attachment 8424880 [details] [diff] [review]
Addressed nits from mfinkle's r+

Removing the approval per jhugman. He'll re-request once he has a branch patch.
Attachment #8424880 - Flags: approval-mozilla-aurora+
Flags: needinfo?(jhugman)
[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: 
Testing completed (on m-c, etc.): 
On M-C for a week.
Risk to taking this patch (and alternatives if risky): 
String or IDL/UUID changes made by this patch:

This was previously approved, but without a branch patch. This patch is that branch patch.
Attachment #8435876 - Flags: approval-mozilla-aurora?
Attachment #8435876 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Verified as fixed using the steps from comment 1 and comment 4.
Builds:
Firefox for Android 31 Beta 6;
Firefox for Android 32.0a1 (2014-07-01);
Devices:
Nexus 4 (Android 4.4.2);
Asus Transformer Pad TF300T (Android 4.2.1).
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.