Closed
Bug 837821
Opened 12 years ago
Closed 12 years ago
Possible startup crash with accessing nsWindow before it exists
Categories
(Firefox for Android Graveyard :: General, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 21
People
(Reporter: kats, Assigned: kats)
References
Details
Attachments
(2 files, 5 obsolete files)
1.44 KB,
patch
|
gbrown
:
review+
|
Details | Diff | Splinter Review |
34.32 KB,
patch
|
glandium
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•12 years ago
|
||
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.
Assignee | ||
Comment 2•12 years ago
|
||
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).
Assignee: nobody → bugmail.mozilla
Attachment #709904 -
Flags: review?(mh+mozilla)
Assignee | ||
Comment 3•12 years ago
|
||
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.
Attachment #709905 -
Flags: review?(gbrown)
Assignee | ||
Comment 4•12 years ago
|
||
Try push at https://tbpl.mozilla.org/?tree=Try&rev=11455396b5c8
Comment 5•12 years ago
|
||
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.
Attachment #709905 -
Flags: review?(gbrown) → review+
Comment 6•12 years ago
|
||
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.
Attachment #709904 -
Flags: review?(mh+mozilla) → review-
Comment 7•12 years ago
|
||
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.
Updated•12 years ago
|
Assignee: bugmail.mozilla → mh+mozilla
Updated•12 years ago
|
Assignee: mh+mozilla → bugmail.mozilla
Comment 8•12 years ago
|
||
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.
Attachment #710162 -
Flags: review?(bugmail.mozilla)
Updated•12 years ago
|
Attachment #710126 -
Attachment is obsolete: true
Comment 9•12 years ago
|
||
Waldo is probably more familiar with such magic ; it's somehow similar to some of the constructs in MFBT.
Attachment #710169 -
Flags: review?(jwalden+bmo)
Updated•12 years ago
|
Attachment #710162 -
Attachment is obsolete: true
Attachment #710162 -
Flags: review?(bugmail.mozilla)
Updated•12 years ago
|
Assignee: bugmail.mozilla → mh+mozilla
Updated•12 years ago
|
Assignee: mh+mozilla → bugmail.mozilla
Assignee | ||
Comment 10•12 years ago
|
||
This version is rebased on top of the patches in bug 794982.
Attachment #709904 -
Attachment is obsolete: true
Attachment #710364 -
Flags: review?(mh+mozilla)
Comment 11•12 years ago
|
||
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.
Attachment #710364 -
Flags: review?(mh+mozilla)
Assignee | ||
Comment 12•12 years ago
|
||
Rebased on top of bug 794982.
Attachment #710364 -
Attachment is obsolete: true
Attachment #711314 -
Flags: review?(mh+mozilla)
Assignee | ||
Comment 13•12 years ago
|
||
Comment on attachment 710169 [details] [diff] [review] Add some macro magic for GeckoAppShell wrappers This patch is obsoleted by bug 794982 now.
Attachment #710169 -
Attachment is obsolete: true
Attachment #710169 -
Flags: review?(jwalden+bmo)
Comment 14•12 years ago
|
||
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')
Attachment #711314 -
Flags: review?(mh+mozilla) → review+
Assignee | ||
Comment 15•12 years ago
|
||
(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•12 years ago
|
||
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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
Assignee | ||
Comment 17•12 years ago
|
||
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•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/489fec58be7e https://hg.mozilla.org/mozilla-central/rev/c522894cc83f
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 21
Updated•4 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•