Closed Bug 1359618 Opened 7 years ago Closed 7 years ago

Prevent LayerView from accessing the compositor until UiCompositorControllerChild is open

Categories

(Firefox for Android Graveyard :: Toolbar, defect, P3)

Product:
Firefox for Android Graveyard
Component:
Toolbar
Version:
55 Branch
Platform:
All
Android
Type:
defect

Tracking

(firefox53 unaffected, firefox54 unaffected, firefox55 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 55
Tracking Flags:
Tracking Status
firefox53 --- unaffected
firefox54 --- unaffected
firefox55 --- fixed

People

(Reporter: rbarker, Assigned: rbarker)

References

Details

(Keywords: regression)

Attachments

(1 file)

Bug 1359618 - Prevent LayerView from accessing the compositor until UiCompositorControllerChild is open
59 bytes, text/x-review-board-request
jchen
: review+
Details 
It is possible when using custom tabs for the LayerView to attempt to resume the compositor before the UiCompositorController has been initialized. This can cause dead tabs on release builds and asserts on debug builds. The LayerView needs to be updated to defer resuming the compositor until IPC has been initialized.
Assignee: nobody → rbarker
Blocks: dynamic-toolbar-3
Comment on attachment 8861710 [details]
Bug 1359618 - Prevent LayerView from accessing the compositor until UiCompositorControllerChild is open

https://reviewboard.mozilla.org/r/133694/#review136600

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/gfx/LayerView.java:105
(Diff revision 1)
>                  mCompositor.sendToolbarAnimatorMessage(message);
>              }
>          });
>      }
>  
> +    @WrapForJNI(stubName = "CompositorCreated", calledFrom = "ui")

Don't bother with `stubName`, just use `IsCompositorReady`

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/gfx/LayerView.java:110
(Diff revision 1)
> +    @WrapForJNI(stubName = "CompositorCreated", calledFrom = "ui")
> +    /* package */ boolean isCompositorReady() {
> +        if (mCompositorCreated && mCompositorControllerOpen) {
> +            return true;
> +        } else if (mCompositorCreated) {
> +            // isCompositorReady() may be called from either ui thread or main thread so dispatch to ui thread.

This is not what the `@WrapForJNI` annotation indicates (`calledFrom = "ui"`), which one is right? I think I only see usage from the UI thread.

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/gfx/LayerView.java:257
(Diff revision 1)
>                      listener.drawFinished();
>                  }
>                  break;
>              case COMPOSITOR_CONTROLLER_OPEN:
> -                mToolbarAnimator.notifyCompositorControllerOpen();
> +                // It is possible to get this message multiple times. Only act on it if we didn't know the compositor controller was open
> +                if (!mCompositorControllerOpen) {

if (mCompositorControllerOpen) {
        break;
    }
Comment on attachment 8861710 [details]
Bug 1359618 - Prevent LayerView from accessing the compositor until UiCompositorControllerChild is open

https://reviewboard.mozilla.org/r/133694/#review136600

> Don't bother with `stubName`, just use `IsCompositorReady`

Okay, I just left the original stub name so I wouldn't have to change nsWindow.cpp but I can change it there.

> This is not what the `@WrapForJNI` annotation indicates (`calledFrom = "ui"`), which one is right? I think I only see usage from the UI thread.

I misunderstood, I thought the annotation was for when it was called from C++ but I see isCompositorCreated() is only called in the ui thread.
status-firefox53: --- → unaffected
status-firefox54: --- → unaffected
status-firefox55: --- → affected
Keywords: regression
Priority: -- → P3
Hardware: Unspecified → All
Version: unspecified → 55 Branch
(In reply to Randall Barker [:rbarker] from comment #3)
> Comment on attachment 8861710 [details]
> Bug 1359618 - Prevent LayerView from accessing the compositor until
> UiCompositorControllerChild is open
>
> I misunderstood, I thought the annotation was for when it was called from
> C++

Technically yes, but I think it was still a little confusing to have the two lines apparently contradicting each other.
Comment on attachment 8861710 [details]
Bug 1359618 - Prevent LayerView from accessing the compositor until UiCompositorControllerChild is open

https://reviewboard.mozilla.org/r/133694/#review136600

> if (mCompositorControllerOpen) {
>         break;
>     }

I forgot this one. I'll push a new patch.
Comment on attachment 8861710 [details]
Bug 1359618 - Prevent LayerView from accessing the compositor until UiCompositorControllerChild is open

https://reviewboard.mozilla.org/r/133694/#review136924

::: mobile/android/base/java/org/mozilla/gecko/GeckoApp.java:1784
(Diff revision 3)
>              Tab selectedTab = Tabs.getInstance().getSelectedTab();
>              if (selectedTab != null) {
>                  Tabs.getInstance().notifyListeners(selectedTab, Tabs.TabEvents.SELECTED);
>              }
>  
> -            if (GeckoThread.isRunning()) {
> +            if (GeckoThread.isRunning() && mInitialized) {

`mInitialized` is always true here.

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/gfx/LayerView.java:106
(Diff revision 3)
>              }
>          });
>      }
>  
> +    @WrapForJNI(calledFrom = "ui")
> +    /* package */ boolean isCompositorReady() {

I think you should assert on UI thread

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/gfx/LayerView.java
(Diff revision 3)
>          @WrapForJNI(calledFrom = "ui", dispatchTo = "current")
>          /* package */ native void sendToolbarPixelsToCompositor(final int width, final int height, final int[] pixels);
>  
>          @WrapForJNI(calledFrom = "gecko")
>          private void reattach() {
> -            mCompositorCreated = true;

Can you make sure we don't regress bug 1296757 by removing this line?
Attachment #8861710 - Flags: review?(nchen) → review+
Comment on attachment 8861710 [details]
Bug 1359618 - Prevent LayerView from accessing the compositor until UiCompositorControllerChild is open

https://reviewboard.mozilla.org/r/133694/#review136924

> Can you make sure we don't regress bug 1296757 by removing this line?

updateCompositor() will set mCompositorCreated to true if the compositor is actually created. There is some custom tab weirdness where we are reattaching but the compositor has actually never been created so just blindly setting it to true can cause crashes. We might end up with an extra call to the actual create function but it bails out early if LayerManager has already been created.
Comment on attachment 8861710 [details]
Bug 1359618 - Prevent LayerView from accessing the compositor until UiCompositorControllerChild is open

https://reviewboard.mozilla.org/r/133694/#review136924

> `mInitialized` is always true here.

As I said, but mozreivew decided to not publish: With custom tabs, mInialized is not always true here. I was getting intermittent crashes in geckoConnected() because mLayerView was null. It gets allocated in initialized() which is where mInitialized is set to true.
But the line you changed is also inside `initialize()`, no? So `mInitialized` should always be set before that line. `mLayerView` is set inside `onCreate()`.
(In reply to Jim Chen [:jchen] [:darchons] from comment #11)
> But the line you changed is also inside `initialize()`, no? So
> `mInitialized` should always be set before that line. `mLayerView` is set
> inside `onCreate()`.

Okay, I'll re-investigate. I hope I can remember what I was doing to get the crash.
Comment on attachment 8861710 [details]
Bug 1359618 - Prevent LayerView from accessing the compositor until UiCompositorControllerChild is open

https://reviewboard.mozilla.org/r/133694/#review136924

> As I said, but mozreivew decided to not publish: With custom tabs, mInialized is not always true here. I was getting intermittent crashes in geckoConnected() because mLayerView was null. It gets allocated in initialized() which is where mInitialized is set to true.

I'm not sure how I got this so twisted, that extra if check is clearly not necissary. To make matters worse, I can't reproduce the issue I was seeing so maybe something else in the patch has fixed it.
Comment on attachment 8861710 [details]
Bug 1359618 - Prevent LayerView from accessing the compositor until UiCompositorControllerChild is open

https://reviewboard.mozilla.org/r/133694/#review136924

> updateCompositor() will set mCompositorCreated to true if the compositor is actually created. There is some custom tab weirdness where we are reattaching but the compositor has actually never been created so just blindly setting it to true can cause crashes. We might end up with an extra call to the actual create function but it bails out early if LayerManager has already been created.

So I tried following the steps to reproduce bug 1296757 and was not able to reproduce. I'm fairly confident I did not regress it as the function updateCompositor will set the flag to true if is not and the compositor is created.
Comment on attachment 8861710 [details]
Bug 1359618 - Prevent LayerView from accessing the compositor until UiCompositorControllerChild is open

https://reviewboard.mozilla.org/r/133694/#review136924

> I think you should assert on UI thread

This assert caught a bug in DynamicToolbarAnimator.setPinned(). I push a new patch once it is passing try.
Pushed by rbarker@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8c4f14f9c570
Prevent LayerView from accessing the compositor until UiCompositorControllerChild is open r=jchen
https://hg.mozilla.org/mozilla-central/rev/8c4f14f9c570
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox55: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: