Closed Bug 1372777 Opened 7 years ago Closed 7 years ago

Crash in java.lang.NullPointerException: Null native pointer at org.mozilla.gecko.mozglue.GeckoLoader.nativeRun(Native Method), mainly about nsWindow::LayerViewSupport

Categories

(Firefox for Android Graveyard :: General, defect)

Unspecified
Android
defect
Not set
critical

Tracking

(fennec+, firefox54 unaffected, firefox55 fixed, firefox56 fixed)

RESOLVED FIXED
Firefox 56
Tracking Status
fennec + ---
firefox54 --- unaffected
firefox55 --- fixed
firefox56 --- fixed

People

(Reporter: ting, Assigned: rbarker)

References

Details

(Keywords: crash)

Crash Data

Attachments

(1 file)

This bug was filed from the Socorro interface and is 
report bp-75449190-1f05-43a6-8348-d1cf60170613.
=============================================================
Top #2 of Nightly 20170611100331 on Android, 17 reports from 15 installations. There are 359 reports in the past week.
The frame 0 is mainly:

  mozilla::jni::detail::ProxyNativeCall<nsWindow::LayerViewSupport, mozilla::java::LayerView::Compositor, false, false, const mozilla::jni::Ref<mozilla::jni::Object, _jobject*>&, const mozilla::jni::Ref<mozilla::jni::Object, _jobject*>&>::operator()

and bp-c554f229-b9d6-406b-b283-657550170607 has:

  nsWindow::LayerViewSupport::SyncResumeResizeCompositor(mozilla::jni::LocalRef<mozilla::java::LayerView::Compositor> const&, int, int, mozilla::jni::Ref<mozilla::jni::Object, _jobject*> const&)::OnResumedEvent::Run()

in frame 0.
Summary: Crash in java.lang.NullPointerException: Null native pointer at org.mozilla.gecko.mozglue.GeckoLoader.nativeRun(Native Method) → Crash in java.lang.NullPointerException: Null native pointer at org.mozilla.gecko.mozglue.GeckoLoader.nativeRun(Native Method), mainly about nsWindow::LayerViewSupport
Randall can you investigate?
Assignee: nobody → rbarker
tracking-fennec: --- → +
Flags: needinfo?(rbarker)
Flags: needinfo?(rbarker)
It looks to me like a Java object that has a native pointer attached to it is being dispatched to a different thread. It would seem in the time it takes to dispatch to the other thread, the native pointer is being detached from the java object so a null pointer gets returned in the dispatch. I'll put some if checks to prevent this and see if that fixes the issue.
Comment on attachment 8879272 [details]
Bug 1372777 - Check that the native pointer has not been detached from the java object when dispatched to different thread

https://reviewboard.mozilla.org/r/150562/#review155764
Attachment #8879272 - Flags: review?(esawin) → review+
Pushed by rbarker@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8e1de9b86753
Check that the native pointer has not been detached from the java object when dispatched to different thread r=esawin
https://hg.mozilla.org/mozilla-central/rev/8e1de9b86753
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
Comment on attachment 8879272 [details]
Bug 1372777 - Check that the native pointer has not been detached from the java object when dispatched to different thread

https://reviewboard.mozilla.org/r/150562/#review157374

::: widget/android/jni/Natives.h:407
(Diff revision 1)
>      template<bool Static, bool ThisArg, size_t... Indices>
>      typename mozilla::EnableIf<!Static && ThisArg, void>::Type
>      Call(const typename Owner::LocalRef& inst,
>           mozilla::IndexSequence<Indices...>) const
>      {
> -        Impl* const impl = NativePtr<Impl>::Get(inst);
> +        if (Impl* const impl = NativePtr<Impl>::Get(inst)) {

This was designed to crash if we don't have a valid pointer here. Instead of ignoring null pointers, we should take care to free pointers after making sure they are no longer used.
(In reply to (pto) Jim Chen [:jchen] [:darchons] from comment #9)
> Comment on attachment 8879272 [details]
> Bug 1372777 - Check that the native pointer has not been detached from the
> java object when dispatched to different thread
> 
> https://reviewboard.mozilla.org/r/150562/#review157374
> 
> ::: widget/android/jni/Natives.h:407
> (Diff revision 1)
> >      template<bool Static, bool ThisArg, size_t... Indices>
> >      typename mozilla::EnableIf<!Static && ThisArg, void>::Type
> >      Call(const typename Owner::LocalRef& inst,
> >           mozilla::IndexSequence<Indices...>) const
> >      {
> > -        Impl* const impl = NativePtr<Impl>::Get(inst);
> > +        if (Impl* const impl = NativePtr<Impl>::Get(inst)) {
> 
> This was designed to crash if we don't have a valid pointer here. Instead of
> ignoring null pointers, we should take care to free pointers after making
> sure they are no longer used.

I think the pointer was valid when it was dispatched. But between the dispatch to a different thread and actually being invoked it seems the pointer gets removed. I can't reproduce so I had to guess.
Please request Beta approval on this when you get a chance.
Flags: needinfo?(rbarker)
Jim, did you have any comments on this patch before I request uplift to beta?
Flags: needinfo?(nchen)
I think it's okay to uplift the current patch to beta. I'll probably work on this more for nightly.
Flags: needinfo?(nchen)
Comment on attachment 8879272 [details]
Bug 1372777 - Check that the native pointer has not been detached from the java object when dispatched to different thread

Approval Request Comment
[Feature/Bug causing the regression]: Intermittent bug, unclear what caused it to start manifesting. 
[User impact if declined]: Possible random crashes when shutting down.
[Is this code covered by automated tests?]: Almost all tests go through this code but there are no explicit tests. 
[Has the fix been verified in Nightly?]: no
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: no
[Why is the change risky/not risky?]: just adds a nullptr check to three different places in code.
[String changes made/needed]: none.
Flags: needinfo?(rbarker)
Attachment #8879272 - Flags: approval-mozilla-beta?
Comment on attachment 8879272 [details]
Bug 1372777 - Check that the native pointer has not been detached from the java object when dispatched to different thread

fennec crash fix, beta55+
Attachment #8879272 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.