837821 - Possible startup crash with accessing nsWindow before it exists

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

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

See https://bugzilla.mozilla.org/show_bug.cgi?id=785597#c43 - when I uplifted that patch to aurora it made everything orange. Some investigation seems to indicate that the call to GeckoAppShell.scheduleComposite happens very early (before libxul is loaded) and the AndroidJNI.cpp call to nsWindow::ScheduleComposite resolves to 0x0, causing the fault. I suspect this may be something that occurs in the wild as well.

Comment 1

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

Confirmed that inserting a call to GeckoAppShell.renderRequested() before GeckoAppShell.loadGeckoLibs is called by GeckoThread triggers a crash. *sigh*

I can easily fix it for this specific case, but I'm wondering if there's a more general way to catch these scenarios where stuff in libxul is referenced before libxul is loaded. Or at least make it more obvious in logcat; this may be the cause of other intermittent failures as well.

Comment 2

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

Attachment: Throw UnsupportedOperationException instead of crashing

This is a lot of copypasta but fixing that up is bug 794982 (incidentally also assigned to me, but that I haven't done anything on for a while).

Comment 3

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

Attachment: Guard against calling scheduleComposite too early

This may be the cause of some startup crashes and intermittent test failures. Even if this specific call isn't, the exceptions stacks from the first patch should show up in logcat and be useful.

Comment 4

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

Try push at https://tbpl.mozilla.org/?tree=Try&rev=11455396b5c8

Comment 5

[Geoff Brown [:gbrown]]

Comment on attachment 709905 [details] [diff] [review]
Guard against calling scheduleComposite too early

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

LGTM. 

Consider logging the exception (or some other, less disturbing message) in case there are unforeseen consequences of this condition.

Comment 6

[Mike Hommey [:glandium]]

Comment on attachment 709904 [details] [diff] [review]
Throw UnsupportedOperationException instead of crashing

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

I think it's time for a little more macro magic in there.

Comment 7

[Mike Hommey [:glandium]]

Attachment: Add some macro magic for GeckoAppShell wrappers

This needs some cleanup and comments, but it allows to get rid of all the repetitions. I validated the preprocessed code is strictly identical to what it currently is, except for "JNIEnv *jenv, jclass jc" used instead of "JNIEnv *, jclass" in the typedefs.

Comment 8

[Mike Hommey [:glandium]]

Attachment: Add some macro magic for GeckoAppShell wrappers

This still expands to the same thing, but is commented and should be a bit clearer. With this patch, there's only one place to modify for all wrappers.

Comment 9

[Mike Hommey [:glandium]]

Attachment: Add some macro magic for GeckoAppShell wrappers

Waldo is probably more familiar with such magic ; it's somehow similar to some of the constructs in MFBT.

Comment 10

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

Attachment: Throw UnsupportedOperationException instead of crashing (v2)

This version is rebased on top of the patches in bug 794982.

Comment 11

[Mike Hommey [:glandium]]

Comment on attachment 710364 [details] [diff] [review]
Throw UnsupportedOperationException instead of crashing (v2)

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

Awaiting changes from bug 794982.

Comment 12

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

Attachment: Throw UnsupportedOperationException instead of crashing (v3)

Rebased on top of bug 794982.

Comment 13

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

Comment on attachment 710169 [details] [diff] [review]
Add some macro magic for GeckoAppShell wrappers

This patch is obsoleted by bug 794982 now.

Comment 14

[Mike Hommey [:glandium]]

Comment on attachment 711314 [details] [diff] [review]
Throw UnsupportedOperationException instead of crashing (v3)

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

::: mobile/android/base/jni-generator.py
@@ +11,5 @@
>  extern "C" NS_EXPORT %(returnType)s JNICALL
>  %(functionName)s(%(parameterList)s) {
> +    if (!f_%(functionName)s) {
> +        arg0->ThrowNew(arg0->FindClass("java/lang/UnsupportedOperationException"),
> +                       "Cannot call %(functionName)s before it is loaded");

My only concern is the resulting size of .rodata, with all these strings.

@@ +64,5 @@
>              match = re.match(paramsRegex, line)
>              if match:
>                  paramTypes = re.split('\s*,\s*', match.group(1))
>                  paramNames = ['arg%d' % i for i in range(0, len(paramTypes))]
> +                returnValue = ''

Since you're not leaving it unset in any branch where you don't throw, you can skip this.

@@ +67,5 @@
>                  paramNames = ['arg%d' % i for i in range(0, len(paramTypes))]
> +                returnValue = ''
> +                if returnType == 'jobject':
> +                    returnValue = 'NULL'
> +                elif returnType == 'jint' or returnType == 'jfloat':

returnType in ('jint', 'jfloat')

Comment 15

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

(In reply to Mike Hommey [:glandium] from comment #14)
> My only concern is the resulting size of .rodata, with all these strings.

I could just use a generic "function called before load" string (without the function name) if that'll help. The java stacktrace of the thrown exception will point to the right function.

> >                  paramNames = ['arg%d' % i for i in range(0, len(paramTypes))]
> > +                returnValue = ''
> 
> Since you're not leaving it unset in any branch where you don't throw, you
> can skip this.

It doesn't set later if returnType is 'void'. I could add an explicit branch for that case and not define it here if you prefer.

> > +                elif returnType == 'jint' or returnType == 'jfloat':
> 
> returnType in ('jint', 'jfloat')

Ok, will do.

Comment 16

[Mike Hommey [:glandium]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #15)
> It doesn't set later if returnType is 'void'. I could add an explicit branch
> for that case and not define it here if you prefer.

Ah, oversigned on my part. But yeah, it might be clearer as
  if returnType == foo:
    returnValue = foo
  elif returnType == bar:
    returnValue = bar
  elif returnType == void:
    returnValue = baz
  else:
    raise

Comment 17

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

I changed the string to be the same for all functions, that saved ~3k in libmozglue.so. Also did the other changes mentioned above.

https://hg.mozilla.org/integration/mozilla-inbound/rev/489fec58be7e
https://hg.mozilla.org/integration/mozilla-inbound/rev/c522894cc83f

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/489fec58be7e
https://hg.mozilla.org/mozilla-central/rev/c522894cc83f