browser.windows.create() using tabId and 'panel' type produces shifted active areas
Categories
(Core :: Panning and Zooming, defect, P1)
Tracking
()
Tracking | Status | |
---|---|---|
firefox-esr60 | --- | unaffected |
firefox-esr68 | --- | wontfix |
firefox68 | --- | wontfix |
firefox69 | --- | wontfix |
firefox70 | --- | verified |
People
(Reporter: vvendigo, Assigned: botond)
References
(Blocks 2 open bugs, Regression)
Details
(Keywords: regression)
Attachments
(4 files, 1 obsolete file)
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:67.0) Gecko/20100101 Firefox/67.0
Steps to reproduce:
In my webextension I want to move tab into the new panel window.
It worked just fine for last two years.
Minimal example:
browser.tabs.query({currentWindow: true}).then((tabs) => {
browser.windows.create({
tabId: tabs[tabs.length-1].id,
type: "detached_panel",
})
});
Actual results:
Resulting window document has all active areas (of inputs, hrefs, buttons etc.) shifted downwards (by height of source window's address/tab bars I guess).
I observed some textarea rendering error, other elements are looking OK.
The same problem occurs when I am using 'popup' or 'panel' window type.
Expected results:
Panel window containing document with properly positioned active areas. :)
Comment 1•5 years ago
|
||
Could you please add an example extension that encapsulates the minimal example and also reproduces the problem?
From comment 0, I assume this is a regression, could you please state what is the last version it worked properly on? Your UA states Fx67, but I'm uncertain if the problem is in Fx67 or not. The OSes affected (to your knowledge) would probably help as well.
Alternatively, you could help us more in actually finding the regression range (if this is a regression as your comment implies: "It worked just fine for last two years.") -> https://mozilla.github.io/mozregression/
Minimal bug reproducing webextension example.
I attached example extension.
It affects Windows and Linux version of FF. (Didn't tested other.)
I am not sure in which version problem started. Seems like it was 67 or 68. First bugreport from our users arrived someday last week (2019-07-8 - 2019-07-12). I am not in direct touch with them.
And one random observation: the bug doesn't affect tab containing 'about:debugging'.
Updated•5 years ago
|
Updated•5 years ago
|
Comment 4•5 years ago
|
||
str |
The previous test case couldn't be loaded, so I created a new test case and used it to bisect the issue. The same test case can also be used to verify the bug.
STR:
- Load the extension, e.g. via
about:debugging
. - The extension will open a new tab. Click on the button to move it to a separate window.
- Try to focus the input field (or the button, or select text).
Expected:
- The input field can be focused by clicking on it.
Actual:
- The input field can only be focused by clicking below it.
Comment 5•5 years ago
|
||
Comment on attachment 9078360 [details]
Simple tab to panel moving extension
This test case has an invalid manifest file. The homepage_url
field cannot be empty. I've created a reduced test case and attached it in comment 4.
Updated•5 years ago
|
Comment 6•5 years ago
|
||
This is not a bug in extensions code. ni? owner of bug 1530661 to investigate later.
The relation with extensions is: https://searchfox.org/mozilla-central/rev/40e889be8ff926e32f7567957f4c316f14f6fbef/browser/components/extensions/parent/ext-windows.js#237,299-341
The tab parameter is used in: https://searchfox.org/mozilla-central/rev/40e889be8ff926e32f7567957f4c316f14f6fbef/browser/base/content/browser.js#1598-1599
Updated•5 years ago
|
Updated•5 years ago
|
Assignee | ||
Comment 7•5 years ago
|
||
Setting status flags based on the regression range.
In a debug build, an assertion fails at https://searchfox.org/mozilla-central/rev/0ffa9e372df56c95547fed9c3433ddec4fbf6f11/gfx/layers/ipc/CompositorBridgeParent.cpp#1704 with the stack:
#0 0x00007fc8da517476 in mozilla::layers::CompositorBridgeParent::RecvAdoptChild(mozilla::layers::LayersId const&) (this=0x7fc8b7405800,
child=...) at /opt/Projects/gecko/gfx/layers/ipc/CompositorBridgeParent.cpp:1704
#1 0x00007fc8d9321419 in mozilla::layers::PCompositorBridgeParent::OnMessageReceived(IPC::Message const&) (this=0x7fc8b7405800, msg__=..
.) at PCompositorBridgeParent.cpp:1092
#2 0x00007fc8d932c565 in mozilla::layers::PCompositorManagerParent::OnMessageReceived(IPC::Message const&) (this=0x7fc8c5507000, msg__=.
..) at PCompositorManagerParent.cpp:200
#3 0x00007fc8d91ad20f in mozilla::ipc::MessageChannel::DispatchAsyncMessage(mozilla::ipc::ActorLifecycleProxy*, IPC::Message const&) (th
is=0x7fc8c5507120, aProxy=0x7fc8c7a2f580, aMsg=...) at /opt/Projects/gecko/ipc/glue/MessageChannel.cpp:2184
#4 0x00007fc8d91ab9dc in mozilla::ipc::MessageChannel::DispatchMessage(IPC::Message&&) (this=0x7fc8c5507120, aMsg=...) at /opt/Projects/
gecko/ipc/glue/MessageChannel.cpp:2108
#5 0x00007fc8d91ac27c in mozilla::ipc::MessageChannel::RunMessage(mozilla::ipc::MessageChannel::MessageTask&) (this=0x7fc8c5507120, aTas
k=...) at /opt/Projects/gecko/ipc/glue/MessageChannel.cpp:1955
#6 0x00007fc8d91ac815 in mozilla::ipc::MessageChannel::MessageTask::Run() (this=0x7fc8afa4dc50) at /opt/Projects/gecko/ipc/glue/MessageC
hannel.cpp:1986
#7 0x00007fc8d90c08a9 in MessageLoop::RunTask(already_AddRefed<nsIRunnable>) (this=0x7fc8c84fdca8, aTask=...) at /opt/Projects/gecko/ipc
/chromium/src/base/message_loop.cc:442
#8 0x00007fc8d90c1066 in MessageLoop::DeferOrRunPendingTask(MessageLoop::PendingTask&&) (this=0x7fc8c84fdca8, pending_task=...) at /opt/
Projects/gecko/ipc/chromium/src/base/message_loop.cc:450
#9 0x00007fc8d90c12a0 in MessageLoop::DoWork() (this=0x7fc8c84fdca8) at /opt/Projects/gecko/ipc/chromium/src/base/message_loop.cc:523
#10 0x00007fc8d90c2138 in base::MessagePumpDefault::Run(base::MessagePump::Delegate*) (this=0x7fc8ce135190, delegate=0x7fc8c84fdca8) at /
opt/Projects/gecko/ipc/chromium/src/base/message_pump_default.cc:35
#11 0x00007fc8d90c06ef in MessageLoop::RunInternal() (this=0x7fc8c84fdca8) at /opt/Projects/gecko/ipc/chromium/src/base/message_loop.cc:3
15
#12 0x00007fc8d90c0665 in MessageLoop::RunHandler() (this=0x7fc8c84fdca8) at /opt/Projects/gecko/ipc/chromium/src/base/message_loop.cc:30
8
#13 0x00007fc8d90c061a in MessageLoop::Run() (this=0x7fc8c84fdca8) at /opt/Projects/gecko/ipc/chromium/src/base/message_loop.cc:290
#14 0x00007fc8d90e23a8 in base::Thread::ThreadMain() (this=0x7fc8ce1f7470) at /opt/Projects/gecko/ipc/chromium/src/base/thread.cc:192
#15 0x00007fc8d90c772e in ThreadFunc(void*) (closure=0x7fc8ce1f7470) at /opt/Projects/gecko/ipc/chromium/src/base/platform_thread_posix.c
c:40
#16 0x00007fc8f02266db in start_thread (arg=0x7fc8c84fe700) at pthread_create.c:463
#17 0x00007fc8ef40c88f in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:95
It looks like this needs a look from the graphics team. The non-debug build manifestation is likely just fallout from executing past this assertion.
Assignee | ||
Comment 9•5 years ago
|
||
(In reply to Henri Sivonen (:hsivonen) from comment #8)
The non-debug build manifestation is likely just fallout from executing past this assertion.
I don't think so, as this assertion occurs even in builds before the landing of the regressing bug.
This assertion firing is probably still worth investigating (in a separate bug), but it doesn't look like it's the cause of this bug.
Assignee | ||
Updated•5 years ago
|
Assignee | ||
Updated•5 years ago
|
Assignee | ||
Comment 10•5 years ago
|
||
Henri, do you think we should consider backing out bug 1530661 until this is resolved? It's a Fission-motivated change that is breaking non-Fission scenarios.
(In reply to Botond Ballo [:botond] from comment #10)
Henri, do you think we should consider backing out bug 1530661 until this is resolved? It's a Fission-motivated change that is breaking non-Fission scenarios.
Backing it out would break Fission pretty badly, so I think backing out would be bad.
(In reply to Botond Ballo [:botond] from comment #9)
(In reply to Henri Sivonen (:hsivonen) from comment #8)
The non-debug build manifestation is likely just fallout from executing past this assertion.
I don't think so, as this assertion occurs even in builds before the landing of the regressing bug.
This assertion firing is probably still worth investigating (in a separate bug), but it doesn't look like it's the cause of this bug.
I commented out the assertion, and then another more serious-looking assertion fires:
https://searchfox.org/mozilla-central/rev/3366c3d24f1c3818df37ec0818833bf085e41a53/gfx/layers/ipc/CompositorBridgeParent.cpp#1749
I think this needs investigation by someone on the graphics team who, unlike me, understands how APZ and the compositor relate to popups and who can assess the implications of these pre-existing assertion failures on this code path.
(In reply to Henri Sivonen (:hsivonen) from comment #12)
I commented out the assertion, and then another more serious-looking assertion fires:
https://searchfox.org/mozilla-central/rev/3366c3d24f1c3818df37ec0818833bf085e41a53/gfx/layers/ipc/CompositorBridgeParent.cpp#1749
Notably, the comment above the assertion says:
// We don't support moving a child from an APZ-enabled compositor to a
// APZ-disabled compositor. The mOptions assertion above should already
// ensure this, since APZ-ness is one of the things in mOptions. Note
// however it is possible for mApzUpdater to be non-null here with
// oldApzUpdater null, because the child may not have been previously
// composited.
Which hints at a deeper non-Fission bug with popup creation being the root cause and the Fission change merely made it visible.
(In reply to Botond Ballo [:botond] from comment #10)
Henri, do you think we should consider backing out bug 1530661 until this is resolved?
The code from that patch was originally behind a pref on Windows:
https://hg.mozilla.org/mozilla-central/rev/91d0c9066fd1#l5.55
I guess we could re-insert a pref check for fission.autostart
at that point of the code as band-aid.
(In reply to Henri Sivonen (:hsivonen) from comment #14)
I guess we could re-insert a pref check for
fission.autostart
at that point of the code as band-aid.
Filed as bug 1574089.
Assignee | ||
Comment 16•5 years ago
|
||
(In reply to Henri Sivonen (:hsivonen) from comment #15)
(In reply to Henri Sivonen (:hsivonen) from comment #14)
I guess we could re-insert a pref check for
fission.autostart
at that point of the code as band-aid.Filed as bug 1574089.
That sounds like a good plan, thanks.
As dicussed with Jessie, we'll look at this for Fission M5.
Comment 17•5 years ago
|
||
What's likely happening here is that the tab is getting moved from an APZ-enabled compositor to an APZ-disabled compositor (as evidenced by the assertions getting tripped) which is something we currently don't properly support in the code. In non-debug builds we proceed anyway, and the tab get stuck with stale matrices, because there would be nothing in the new APZ-disabled compositor updating those matrices. So a workaround might be to modify the MatrixMessage
type to carry a Maybe<Matrix>
instead of a Matrix
, and then if a tab is moved from APZ-enabled to APZ-disabled, we use that to clear the matrix on the content side. This would be done by something like this:
if (oldApzUpdater && !mApzUpdater) {
// workaround for bug 1565525
nsTArray<MatrixMessage> clear;
clear.AppendElement(MatrixMessage(Nothing(), child));
if (GeckoContentController* cont = GetGeckoContentControllerForRoot(child)) {
cont->NotifyLayerTransforms(clear);
}
}
at this point. On the content we'd set the matrix to Nothing()
here instead of setting it to Some(matrix)
.
Also given that the STR is a legitimate use case, we need to modify the graphics stack to properly support moving a tab from APZ-enabled to APZ-disabled compositors (and vice-versa) so this workaround isn't so much a workaround as a step in that direction. Another option would be to enable APZ on compositors for panel windows which may or may not be covered by bug 1493208 (I've lost track of the specific scenarios that bug covers).
Assignee | ||
Comment 18•5 years ago
|
||
(In reply to Kartikaya Gupta (email:kats@mozilla.com) (away until Feb-2020) from comment #17)
Another option would be to enable APZ on compositors for panel windows which may or may not be covered by bug 1493208 (I've lost track of the specific scenarios that bug covers).
I believe it would be covered by bug 1493208, since "panel" and "popup" are equivalent.
Updated•5 years ago
|
Assignee | ||
Comment 19•5 years ago
|
||
(In reply to Kartikaya Gupta (email:kats@mozilla.com) (away until Feb-2020) from comment #17)
So a workaround might be to modify the
MatrixMessage
type to carry aMaybe<Matrix>
instead of aMatrix
, and then if a tab is moved from APZ-enabled to APZ-disabled, we use that to clear the matrix on the content side.
I tried to implement this, but ran into the issue that if we don't have APZ, we don't have a GeckoContentController
either.
Assignee | ||
Comment 20•5 years ago
|
||
(In reply to Botond Ballo [:botond] from comment #19)
I tried to implement this, but ran into the issue that if we don't have APZ, we don't have a
GeckoContentController
either.
I guess this can be worked around by using the GeckoContentController
from the old compositor.
Assignee | ||
Comment 21•5 years ago
|
||
Ok, I've implemented the suggested fix locally and it does fix the STR from comment 4.
Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a87fc4698fdc88821ab1c5024a316639dd803abc
Comment 22•5 years ago
|
||
Nice :) and yeah, the controller from the old APZ is the right one to use. I didn't realize it would change but that makes sense.
Assignee | ||
Comment 23•5 years ago
|
||
Assignee | ||
Comment 24•5 years ago
|
||
Depends on D42564
Assignee | ||
Comment 25•5 years ago
|
||
Depends on D42565
Comment 26•5 years ago
|
||
Comment 27•5 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/a7651a98ea51
https://hg.mozilla.org/mozilla-central/rev/6b1a0a29644e
https://hg.mozilla.org/mozilla-central/rev/71b6b2fc33cb
Updated•5 years ago
|
Updated•5 years ago
|
Updated•5 years ago
|
Assignee | ||
Comment 28•5 years ago
|
||
Thanks for the fix suggestion Kats! I had to extend it a bit in bug 1576524 for WebRender but otherwise it seems to be working well.
To the bug reporter, sorry we couldn't get the fix into 69, it was too close to the release date to take a somewhat risky fix.
Assignee | ||
Comment 29•5 years ago
|
||
(In reply to Kartikaya Gupta (email:kats@mozilla.com) (away until Feb-2020) from comment #17)
Also given that the STR is a legitimate use case, we need to modify the graphics stack to properly support moving a tab from APZ-enabled to APZ-disabled compositors (and vice-versa) so this workaround isn't so much a workaround as a step in that direction.
Filed bug 1579896 for the full fix here.
Updated•5 years ago
|
Comment 30•5 years ago
|
||
I’ve reproduced this issue with Fx 70.0a1 (2019-07-24) on Windows 10 x64 using the information from comment 4.
The issue is verified fixed with Fx 71.0a1 (2019-09-23) and Fx 70.0b9 on Windows 10 x64, macOS 10.13 and Ubuntu 18.04 x64.
Updated•5 years ago
|
Description
•