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)
Tracking
(firefox53 unaffected, firefox54 unaffected, firefox55 fixed)
RESOLVED
FIXED
Firefox 55
Tracking | Status | |
---|---|---|
firefox53 | --- | unaffected |
firefox54 | --- | unaffected |
firefox55 | --- | fixed |
People
(Reporter: rbarker, Assigned: rbarker)
References
Details
(Keywords: regression)
Attachments
(1 file)
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 | ||
Updated•7 years ago
|
Assignee: nobody → rbarker
Assignee | ||
Updated•7 years ago
|
Blocks: dynamic-toolbar-3
Comment hidden (mozreview-request) |
Comment 2•7 years ago
|
||
mozreview-review |
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;
}
Assignee | ||
Comment 3•7 years ago
|
||
mozreview-review-reply |
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.
Updated•7 years ago
|
status-firefox53:
--- → unaffected
status-firefox54:
--- → unaffected
status-firefox55:
--- → affected
Keywords: regression
Priority: -- → P3
Hardware: Unspecified → All
Version: unspecified → 55 Branch
Comment 4•7 years ago
|
||
(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 hidden (mozreview-request) |
Assignee | ||
Comment 6•7 years ago
|
||
mozreview-review-reply |
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 hidden (mozreview-request) |
Comment 8•7 years ago
|
||
mozreview-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
::: 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+
Assignee | ||
Comment 9•7 years ago
|
||
mozreview-review-reply |
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.
Assignee | ||
Comment 10•7 years ago
|
||
mozreview-review-reply |
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.
Comment 11•7 years ago
|
||
But the line you changed is also inside `initialize()`, no? So `mInitialized` should always be set before that line. `mLayerView` is set inside `onCreate()`.
Assignee | ||
Comment 12•7 years ago
|
||
(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.
Assignee | ||
Comment 13•7 years ago
|
||
mozreview-review-reply |
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 hidden (mozreview-request) |
Assignee | ||
Comment 15•7 years ago
|
||
mozreview-review-reply |
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.
Assignee | ||
Comment 16•7 years ago
|
||
mozreview-review-reply |
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.
Comment hidden (mozreview-request) |
Comment 18•7 years ago
|
||
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
Comment 19•7 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
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
•