1408446 - Add-ons that open Windows can get an empty window

Status: RESOLVED WORKSFORME

(Firefox :: Tabbed Browser, defect, P3)

Description

[Gian-Carlo Pascutto [:gcp]]

I am not 100% sure what is going on, but bug 1379587 breaks wxIF: 
https://github.com/gcp/wxif 
https://addons.mozilla.org/en-US/firefox/addon/wxif/

And I bisected it to this:
 9:07.92 INFO: No more inbound revisions, bisection finished.
 9:07.92 INFO: Last good revision: 7e5b28819e5b382bea8cbcccf0c6402fe0e90192
 9:07.92 INFO: First bad revision: 0533dbf2956818762fbbe7dbcfad738fe8383d89
 9:07.92 INFO: Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=7e5b28819e5b382bea8cbcccf0c6402fe0e90192&tochange=0533dbf2956818762fbbe7dbcfad738fe8383d89

It's possible that my add-on doesn't use our APIs correctly, but in any case that bugfix seems to have unintended add-on compatibility effects.

Comment 1

[Florian Quèze [:florian]]

(In reply to Gian-Carlo Pascutto [:gcp] from comment #0)
> I am not 100% sure what is going on, but bug 1379587 breaks wxIF

Can you be a bit more specific about what "breaks"? After installing the add-on in a current Nightly and using its context menu item on a picture, I get https://i.imgur.com/exU8rab.png

Comment 2

[Fisher]

I am the original reporter of the phenomenon. The problem happened on my office pc (Debian linux), I can double check on Monday. Tomorrow I can test again on my home pc and provide more detail (actual OS version, libs, and so on).

I have checked it on my laptop (Win 10, FF 57.0b8) and have no issues.

Comment 3

[Gian-Carlo Pascutto [:gcp]]

I bisected it on Ubuntu 16.04 so the regression might be Linux only.

Comment 4

[Fisher]

(In reply to Gian-Carlo Pascutto [:gcp] from comment #3)
> I bisected it on Ubuntu 16.04 so the regression might be Linux only.

Ehh, sort of false alarm, maybe. On my home linux machine the plugin window is also empty. But after resizing it, even to smaller than the original size was the exif data appears.
Debian, stable, intel & nvidia drivers.

Comment 5

[Gian-Carlo Pascutto [:gcp]]

We shouldn't need to resize the window in order for the content to be visible. If anything that supports that it's not an issue with the plug-ins API use, but with Firefox's paint handling. (But note that even if it were an add-on bug, this would still mean there's compatibility impact.)

Comment 6

[Florian Quèze [:florian]]

I can reproduce on Linux, and indeed resizing makes the window content appear (this likely indicates we are missing the first paint event, and removing the blank attribute when resizing forces a repaint). I'll investigate.

Comment 7

[Florian Quèze [:florian]]

Attachment: set dialog=no

I can only reproduce the bug on Linux, and if I set dialog=no at http://searchfox.org/mozilla-central/rev/0bde08bed9b34fb6a26bcb5829dc3c180db0bb72/browser/components/extensions/ext-windows.js#147 I can no longer reproduce the bug.

I don't understand why setting the 'dialog' feature would eat the first MozAfterPaint event only on Linux, but when setting dialog=no I didn't notice any obvious difference on the appearance of the popup window (I tested only on Mac and Linux).

Kris, do you remember what setting the dialog feature there is expected to do?

Comment 8

[Panos Astithas (he/him) [:past] (please ni?)]

Perhaps the dialog feature causes a GTK top level window instead of a GTK popup window to be used (per bug 1109868 comment 16)?

Comment 9

[Kris Maglione [:kmag]]

(In reply to Florian Quèze [:florian] [:flo] from comment #7)
> Kris, do you remember what setting the dialog feature there is expected to
> do?

It's mostly a hint to the window manager/OS about how the window should be displayed, and generally causes the minimize/maximize buttons to be disabled. But for the most part, we shouldn't really treat them differently internally.

Comment 10

[Kris Maglione [:kmag]]

Comment on attachment 8919298 [details] [diff] [review]
set dialog=no

Review of attachment 8919298 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/extensions/ext-windows.js
@@ +143,5 @@
>            if (createData.type === null || createData.type == "normal") {
>              features.push("dialog=no", "all");
>            } else {
>              // All other types create "popup"-type windows by default.
> +            features.push("dialog=no", "resizable", "minimizable", "centerscreen", "titlebar", "close");

May as well just move this to the base `features` array above.

Comment 11

[Florian Quèze [:florian]]

Attachment: Patch v2

Comment 12

[Panos Astithas (he/him) [:past] (please ni?)]

Andy, if Kris is swamped, is there anyone else available to review this in time for 57?

Comment 13

[Andy McKay]

Kris has reviewed, let me know if we can do anything more to help.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2311fe1447d8
Avoid the 'dialog' features to prevent blank add-on popups on Linux, r=kmag.

Comment 15

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for browser-chrome failures in browser/components/extensions/test/browser/browser_ext_windows.js:

https://hg.mozilla.org/integration/mozilla-inbound/rev/7e6b2c4d23966de1fc39eca586b98a6300796271

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=2311fe1447d86bffef4ae0d9e4433325ff9d9848&filter-resultStatus=usercancel&filter-resultStatus=runnable&filter-resultStatus=retry&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=139776432&repo=mozilla-inbound

[task 2017-10-26T08:01:11.811Z] 08:01:11     INFO - TEST-PASS | browser/components/extensions/test/browser/browser_ext_windows.js | Expect first window to be always on top - Expected: true, Actual: true - 
[task 2017-10-26T08:01:11.813Z] 08:01:11     INFO - Console message: [JavaScript Error: "TypeError: invalid 'in' operand browser" {file: "chrome://browser/content/tabbrowser.xml" line: 2437}]
[task 2017-10-26T08:01:11.814Z] 08:01:11     INFO - _insertBrowser@chrome://browser/content/tabbrowser.xml:2437:1
[task 2017-10-26T08:01:11.815Z] 08:01:11     INFO - getRelatedElement@chrome://browser/content/tabbrowser.xml:7142:11
[task 2017-10-26T08:01:11.817Z] 08:01:11     INFO - set_selectedIndex@chrome://global/content/bindings/tabbox.xml:392:31
[task 2017-10-26T08:01:11.818Z] 08:01:11     INFO - tabs_XBL_Constructor@chrome://global/content/bindings/tabbox.xml:261:13
[task 2017-10-26T08:01:11.819Z] 08:01:11     INFO - @chrome://browser/content/tabbrowser.xml:45:9
[task 2017-10-26T08:01:11.821Z] 08:01:11     INFO - _updateNewTabVisibility@chrome://browser/content/tabbrowser.xml:5817:15
[task 2017-10-26T08:01:11.822Z] 08:01:11     INFO - tabbrowser_XBL_Constructor@chrome://browser/content/tabbrowser.xml:5874:11
[task 2017-10-26T08:01:11.823Z] 08:01:11     INFO - 
[task 2017-10-26T08:01:11.825Z] 08:01:11     INFO - TEST-PASS | browser/components/extensions/test/browser/browser_ext_windows.js | Expect three windows - Expected: 3, Actual: 3 - 
[task 2017-10-26T08:01:11.826Z] 08:01:11     INFO - Buffered messages finished
[task 2017-10-26T08:01:11.828Z] 08:01:11     INFO - TEST-UNEXPECTED-FAIL | browser/components/extensions/test/browser/browser_ext_windows.js | Expect one window - Expected: 1, Actual: 0 - 
[task 2017-10-26T08:01:11.829Z] 08:01:11     INFO - Stack trace:
[task 2017-10-26T08:01:11.830Z] 08:01:11     INFO -     @moz-extension://dbd95383-cbe9-4a75-a8f2-f6f4973705d2/%7B7ac6d032-1cb3-4f86-8a72-3644db806775%7D.js:16:7
[task 2017-10-26T08:01:11.831Z] 08:01:11     INFO -     async*@moz-extension://dbd95383-cbe9-4a75-a8f2-f6f4973705d2/%7B7ac6d032-1cb3-4f86-8a72-3644db806775%7D.js:1:8
[task 2017-10-26T08:01:11.833Z] 08:01:11     INFO -     
[task 2017-10-26T08:01:11.834Z] 08:01:11     INFO - GECKO(2653) | JavaScript error: moz-extension://dbd95383-cbe9-4a75-a8f2-f6f4973705d2/%7B7ac6d032-1cb3-4f86-8a72-3644db806775%7D.js, line 17: TypeError: wins[0] is undefined
[task 2017-10-26T08:01:11.837Z] 08:01:11     INFO - Console message: [JavaScript Error: "TypeError: wins[0] is undefined" {file: "moz-extension://dbd95383-cbe9-4a75-a8f2-f6f4973705d2/%7B7ac6d032-1cb3-4f86-8a72-3644db806775%7D.js" line: 17}]
[task 2017-10-26T08:01:55.649Z] 08:01:55     INFO - Not taking screenshot here: see the one that was previously logged

Comment 16

[Florian Quèze [:florian]]

[Tracking Requested - why for this release]: regression in 57

Comment 17

[Florian Quèze [:florian]]

Attachment: Patch v3

Tree is green with this new version: https://treeherder.mozilla.org/#/jobs?repo=try&revision=95043b716d0a0bbeb3894049c12087fad2897c49

The failures were mostly due to bitrot with the changes from bug 1406379 that landed right after I did the first version of the patch here.

Comment 18

[Liz Henry (:lizzard) (relman/hg->git project)]

This could still make it into Thursday's 57 beta 14 build if we can verify the fix.
Is this likely an issue for Win & Mac or just Linux users?

Comment 19

[Florian Quèze [:florian]]

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #18)

> Is this likely an issue for Win & Mac or just Linux users?

I could only reproduce on Linux.

Comment 20

[Jim Mathies [:jimm]]

Andy, can you help triage for 57? Not sure if we want to push this up.

Comment 21

[Andy McKay]

It seems low risk, however the issue can only be re-produced on Linux. I don't think that's important enough to push into Firefox 57 at this late stage given the number of users we have on Linux.

Comment 22

[Panos Astithas (he/him) [:past] (please ni?)]

Nobody seems to consider this a blocker for 57 and it is taking a long time to land anyway. Setting as wontfix for 57.

Comment 24

[mapx]

Not only Linux, Windows 10 too
https://issues.adblockplus.org/ticket/5817

Comment 25

[Gian-Carlo Pascutto [:gcp]]

(In reply to mix from comment #24)
> Not only Linux, Windows 10 too
> https://issues.adblockplus.org/ticket/5817

I'm not sure, the developer's bug was  reported on Linux as well, the track ticket describes somewhat similar symptoms but the underlying cause might be something entirely different.

Windows was broken around Sept 15, so if on Windows a Nightly from a few days before that works, and afterwards it does not, then it was indeed this bug. (And it should be very very urgently re-triaged)

Comment 26

[Gian-Carlo Pascutto [:gcp]]

Sorry, didn't mean to set the tracking flag without a confirmation that this did indeed regress Windows as well.

Comment 27

[Gian-Carlo Pascutto [:gcp]]

(In reply to Gian-Carlo Pascutto [:gcp] from comment #25)
> Windows was broken around Sept 15, so if on Windows a Nightly from a few


And this should have said "Linux Nightly was broken"...

Comment 28

[Wladimir Palant]

https://issues.adblockplus.org/ticket/5817 is definitely a different bug (window isn't blank, merely not initialized). The blank window being opened is https://issues.adblockplus.org/ticket/5985 and really a Linux-only issue.

Comment 29

[Gian-Carlo Pascutto [:gcp]]

Thanks Wladimir. Removing tracking.

Comment 30

[Marco]

Authenfication .htaccess on Keefox Version 2.0.0.23b5 : https://addons.mozilla.org/en-GB/firefox/addon/keefox/versions/beta?page=1#version-2.0.0.23b5 

https://bugzilla.mozilla.org/attachment.cgi?id=8927797


On On TrashMail.com for Mozilla Firefox : https://addons.mozilla.org/fr/firefox/addon/trashmailnet/

https://bugzilla.mozilla.org/attachment.cgi?id=8927816

Comment 32

[Aetf]

Hi, what's the status of this?

Comment 34

[Javier Serrano Polo]

I do not know if you are convinced with setting "dialog=no".

Information from #1428352: This bug happens when opening a moz-extension: page; loading an http: page works as expected.

Comment 35

[Kris Maglione [:kmag]]

Sorry, I've been meaning to get back to this. I'm OK with setting dialog=no, but I'm concerned about how that breaks the window type queries in the latest version of the patch. We need a better solution to that than changing the tests...

Comment 36

[Javier Serrano Polo]

> I'm concerned about how that breaks the window type queries

Instead of using CHROME_LOCATIONBAR to determine if the type is "popup", a dummy CHROME_POPUP flag should be used. Checking CHROME_OPENAS_DIALOG would also be good. Florian, do you know what I mean?

Comment 37

[Florian Quèze [:florian]]

(In reply to Javier Serrano Polo from comment #36)

> Checking CHROME_OPENAS_DIALOG would also be good.

This is what we were doing without the patch.

> Florian, do you know what I mean?

Not really, no. Feel free to steal the bug if you have ideas about how to fix it.

Comment 38

[Javier Serrano Polo]

Setting "dialog=no" does not fix the bug. If I add "toolbar", "location", or anything that resizes the contents, then the contents are displayed. Should I force a resize?

Comment 39

[sdaniele3]

https://bugzilla.mozilla.org/show_bug.cgi?id=1402110 looks to be a duplicate of this. And from the linked ticket, and some basic testing that I've done, it looks like setting `extensions.webextensions.remote` to true resolves the issue (which is false by default on Linux). Would that be related?

Comment 40

[Javier Serrano Polo]

Setting "extensions.webextensions.remote" does not fix the bug on Firefox 57.

I am tracking the painting code...

Comment 41

[sdaniele3]

I'm NOT saying that the option resolves the bug and nothing further needs to be done. Rather, from my experience, and others, enabling the option causes windows created by extensions to paint, without manually resizing.

Testing info:
elementaryOS (Ubunutu Based)
Firefox 57.0.4
Very simple extension. Opens an internal extension page in a new window when pressing the browser action button.
Clean Profile

After installing the extension and pressing the browser action I get a blank window. Restarting Firefox and the behavior continues. Went to about:config and flipped `extensions.webextensions.remote` to true. Restart Firefox. Pressing the browser action I get a window that properly shows the content.

---

I also performed the test on Windows (but in reverse). Although, on the nightly branch rather than Firefox 57.

Windows 7
Firefox 59.0a1 (2018-01-13)
Very simple extension. Opens an internal extension page in a new window when pressing the browser action button.
Clean Profile

After installing the extension and pressing the browser action I get a window with the proper content. Went to about:config and flipped `extensions.webextensions.remote` to false. Restart Firefox. Pressing the browser action I get a blank window.

---

So, maybe my testing is for naught, and I don't understand what I'm seeing. I'm not familiar with Firefox internals but from my external testing, to me, it looks like `remote.webextensions.remote` is doing something in regard to painting windows opened by extensions.

Comment 42

[Javier Serrano Polo]

According to the previous comment, the bug can be reproduced on Windows, thus someone more knowledgeable than me may want to take a look; I may investigate further another day. The resize workaround works, if you are happy with it in the meantime; it is basically:

> window.resizeTo(width, height - 1);
> window.setTimeout(() => {
>   window.resizeTo(width, height);
> }, 0);

Comment 43

[Javier Serrano Polo]

If I invalidate the first child layer of root in nsDisplayList::PaintRoot(), the contents will show. resizeTo() is expected to repaint, no matter if size changed, so I could invalidate that layer whenever resizeTo() is called. Could anyone tell me whether it is acceptable to follow this solution?

Comment 44

[Javier Serrano Polo]

Attachment: Invalidate layer

Here it is my proposal.

Comment 45

[Kris Maglione [:kmag]]

Comment on attachment 8942773 [details] [diff] [review]
Invalidate layer

Review of attachment 8942773 [details] [diff] [review]:
-----------------------------------------------------------------

The nsWindow.cpp changes will need review from a Widget peer.

I'm OK with this as a temporary workaround if they are.

Comment 46

[Florian Quèze [:florian]]

Comment on attachment 8942773 [details] [diff] [review]
Invalidate layer

Review of attachment 8942773 [details] [diff] [review]:
-----------------------------------------------------------------

::: firefox-57.0.3.orig/browser/components/extensions/ext-windows.js
@@ +190,4 @@
>              if (createData.titlePreface) {
>                win.setTitlePreface(createData.titlePreface);
>              }
> +            window.resizeTo(window.outerWidth, window.outerHeight);

window.resizeBy(0, 0); may be slightly faster.

Comment 47

[Javier Serrano Polo]

> window.resizeBy(0, 0); may be slightly faster.

Performance measurements (ms):

window.resizeTo(window.outerWidth, window.outerHeight);
1.715
4.760
2.735
2.005
3.340

window.resizeBy(0, 0);
5.020
6.895
5.990
6.950
6.465

Comment 48

[Javier Serrano Polo]

The patch does not fix the bug in Firefox 58.

Comment 49

[Javier Serrano Polo]

Just for the record, setting "extensions.webextensions.remote" in Firefox 58 shows the content (patch still applied), but it breaks allowScriptsToClose:

> NS_ERROR_NOT_INITIALIZED: Component returned failure code: 0xc1f30001 (NS_ERROR_NOT_INITIALIZED) [nsIMessageSender.sendAsyncMessage]  ext-windows.js:19

Comment 50

[Javier Serrano Polo]

Firefox 58 loads the page later. If I resize one second after the delayed startup, contents show up.

Comment 51

[Javier Serrano Polo]

Attachment: Invalidate layer (2)

This version works with Firefox 58.

Comment 52

[Karl Tomlinson (:karlt)]

Comment on attachment 8946683 [details] [diff] [review]
Invalidate layer (2)

This may be a useful diagnostic patch for narrowing down the cause, but not a
patch for production.

If this is necessary, then it needs more explanation as to why it is
necessary, but I doubt a resize is the appropriate way to invalidate
and I don't know why the extensions component should need to think about
invalidation.

Perhaps this is just triggering an extra MozAfterPaint.

There are existing things that I don't see clearly documented such as what is
expected to trigger the MozAfterPaint after tab-content.js is loaded, or after
documentURI changes while the browser has blank="true".

Comment 53

[Liz Henry (:lizzard) (relman/hg->git project)]

Does the issue happen in 60? You probably should be building and testing against 60 (Nightly)
We are unlikely to fix this in 58 since 58 was released a few weeks ago.

Comment 54

[Javier Serrano Polo]

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #53)
> Does the issue happen in 60?

No idea.

> You probably should be building and testing
> against 60 (Nightly)

Sorry, I cannot afford that right now.

They say this bug can be reproduced in Windows by setting "extensions.webextensions.remote" to false.

Comment 55

[sdaniele3]

I'm able to reproduce on Nightly 60 with Windows (extensions.webextensions.remote = false [NOT THE DEFAULT]).

I don't know which bits are important, but here's everything from "about:buildconfig"

about:buildconfig

Built from https://hg.mozilla.org/mozilla-central/rev/a8e153c55eeee93a11e87d325fb20c644421036f


Build platform
x86_64-pc-mingw32


Build tools
z:/build/build/src/vs2017_15.4.2/VC/bin/Hostx64/x64/cl.exe 	19.11.25547 	-TC -nologo -wd4091 -D_HAS_EXCEPTIONS=0 -W3 -Gy -Zc:inline -Gw -wd4244 -wd4267 -we4553

z:/build/build/src/vs2017_15.4.2/VC/bin/Hostx64/x64/cl.exe 	19.11.25547 	-utf-8 -TP -nologo -w15038 -wd5026 -wd5027 -Zc:sizedDealloc- -wd4091 -wd4577 -D_HAS_EXCEPTIONS=0 -W3 -Gy -Zc:inline -Gw -wd4251 -wd4244 -wd4267 -wd4800 -wd4595 -we4553 -D_SILENCE_TR1_NAMESPACE_DEPRECATION_WARNING -GR- -Zi -O1 -Oi -Oy-


Configure options
MOZ_AUTOMATION=1 'MOZILLABUILD=C:\mozilla-build' --host=x86_64-pc-mingw32 --target=x86_64-pc-mingw32 --enable-update-channel=nightly MOZILLA_OFFICIAL=1 MOZ_PGO=1 WINDOWSSDKDIR=z:/build/build/src/vs2017_15.4.2/SDK MAKECAB=z:/build/build/src/makecab.exe --enable-jemalloc --enable-js-shell RUSTC=z:/build/build/src/rustc/bin/rustc CARGO=z:/build/build/src/rustc/bin/cargo --with-mozilla-api-keyfile=c:/builds/mozilla-desktop-geoloc-api.key --with-google-api-keyfile=c:/builds/gapi.data LLVM_CONFIG=z:/build/build/src/clang/bin/llvm-config --enable-rust-simd MAKE=z:/build/build/src/mozmake.EXE --enable-crashreporter --enable-verify-mar --with-branding=browser/branding/nightly

Comment 56

[Liz Henry (:lizzard) (relman/hg->git project)]

Thanks javier and sdanele3. Florian, does this help? Is this potentially something we want to fix for 59?

Comment 57

[Florian Quèze [:florian]]

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #56)
> Florian, does this help? Is this potentially
> something we want to fix for 59?

I don't expect to fix this myself for 59. I'm considering changing the approach I took in bug 1379587 (the goal would be to eliminate the early and undesired about:blank loads instead of hiding them) in a way that would fix both this bug and bug 1422507. I don't expect my patch to do that to be something I would be comfortable uplifting.

Comment 58

[Mike Conley (:mconley) (:⚙️)]

Hey florian, I just moved this to the [fxperf] tag. Would you mind setting the right priority on this based on its current state?

Comment 59

[Morikko]

I still have the problem with FF59 and the nightly on Fedora 27. 
Every time, I open a popup with a extension page, the page stays blank but the content is loaded. Scripts are executed and hovering an input changes the mouse cursor.

If I resize the window after it was opened (waiting with await), the content is shown.

Comment 60

[jannes.meyer]

I just started having this problem with webextension popups in Firefox 60 on OSX. In Firefox 59 I wasn't affected yet.

Comment 61

[Javier Serrano Polo]

Attachment: Invalidate layer (3)

This version works with Firefox 60. It is essentially the same as previous one.

Comment 63

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I did a bit of debugging of what is presumably the same problem, starting from bug 1425829 comment 15.  I *think* what was going on is that when the iframe is in-process, the display list's optimizations (which build a single display list for the painting within a given process, crossing iframe boundaries) optimize away the opacity:0 subtree (including the iframe and its contents) which causes the code that runs MozAfterPaint in the iframe to not get fired, which led the blank="true" to not get removed.  But I wasn't entirely sure.

Comment 64

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I stopped seeing this bug today (or was it yesterday?), now that bug 1357487 has landed, enabling OOP WebExtensions on Linux.

Comment 66

[thijs]

Can confirm that I no longer see this bug since FF 63.

Comment 67

[dauphine-dev]

I confirm too

Comment 68

[Célestin Matte]

I'm having this bug on Firefox 67.0 on Linux: on an extension I'm developing, pop-up window randomly appears blank when opening it. This does not happen with the same extension on chromium.
The workaround proposed in this duplicate of this bug works: https://bugzilla.mozilla.org/show_bug.cgi?id=1425829#c12