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

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Ting-Yu Chou [:ting] (away)]

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.

Comment 1

[Ting-Yu Chou [:ting] (away)]

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.

Comment 3

[James Willcox (:snorp) ([e-mail]) (he/him)]

Randall can you investigate?

Comment 4

[Randall Barker [: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 5

[Randall Barker [:rbarker]]

Attachment: Bug 1372777 - Check that the native pointer has not been detached from the java object when dispatched to different thread

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

Comment 6

[Eugen Sawin [:esawin]]

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

Comment 7

[Pulsebot]

Pushed by [e-mail]:
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

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/8e1de9b86753

Comment 9

[(inactive) Jim Chen [:jchen] [:darchons]]

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.

Comment 10

[Randall Barker [:rbarker]]

(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.

Comment 11

[Ryan VanderMeulen (PTO, back July 27)]

Please request Beta approval on this when you get a chance.

Comment 12

[Randall Barker [:rbarker]]

Jim, did you have any comments on this patch before I request uplift to beta?

Comment 13

[(inactive) Jim Chen [:jchen] [:darchons]]

I think it's okay to uplift the current patch to beta. I'll probably work on this more for nightly.

Comment 14

[Randall Barker [:rbarker]]

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.

Comment 15

[Julien Cristau [:jcristau]]

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+

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/e8e60131e908