1483033 - Removal of screenshot icon from address bar does not persist across a restart

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, defect, P1)

Description

[Darkspirit]

Attachment: 2018-08-13_23-39-17.mp4

Screencast: mozregression --launch 2018-08-13 -a https://example.com -a about:restartrequired

If example.com has by default a screenshot icon in the address bar, right click on it, click on "Don't Show in address bar" and restart Nightly. It's bad if the screenshot icon is visible again.

mozregression --good 2018-08-10 --bad 2018-08-13 -a https://example.com -a about:restartrequired
> 8:52.01 INFO: Last good revision: 3dc7979622c3ca97221db9320efe1d2c9ca76cb8
> 8:52.01 INFO: First bad revision: b17bf2e7a761071252eb4e65c553ba841dc674cb
> 8:52.01 INFO: Pushlog:
> https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=3dc7979622c3ca97221db9320efe1d2c9ca76cb8&tochange=b17bf2e7a761071252eb4e65c553ba841dc674cb

> b17bf2e7a761	Barry Chen — Bug 1466575 - Update tests to use new Screenshots pageAction id; r=_6a68
> ea0593b7eb05	Barry Chen — Bug 1466575 - Replace Photon pageAction with WebExtension pageAction in Screenshots; r=_6a68

I don't know how I have fixed it in my main profile.

Comment 1

[John Gruen [:jgruen]]

Screenshots users should not be able to notice the removal of Screenshots' bootstrap code. This means that the page action should be in the same place before and after the switch. If the user has previously added it to their search bar via ctrl/command + click it should be there as well in the same position as before. 

For the screenshot button in the context menu, this should stay mixed in with the other webextension context menu items.

Comment 2

[Barry Chen]

I think John's comment (Comment 1) is talking about something a little different: the Screenshots page action should not show up in the URL bar _by default_ at all after the switch.

Comment 3

[Darkspirit]

Both regressed. It is shown by default and removal does not persist.

(In reply to Jan Andre Ikenmeyer [:darkspirit] from comment #0)
> I don't know how I have fixed it in my main profile.
Unspectacular: I removed it and did not restart so far.

Comment 6

[:Felipe Gomes (needinfo for replies!)]

[Tracking Requested - why for this release]:
Screenshot icon always comes back to the address bar even if removed

Comment 7

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Tracked for 63 since it's a recent regression.

Comment 8

[Pascal Chevrel:pascalc]

Ian, could you help finding an owner for this regression? Thanks

Comment 9

[Darkspirit]

(In reply to Pascal Chevrel:pascalc from comment #8)
> Ian, could you help finding an owner for this regression? Thanks

They temporarily backed out the the causing patch in bug 1466575 comment 38, so firefox63 might be set to unaffected?
(This bug's status depends on an outstanding decision whether they wanted to fix it before relanding or not.)

Comment 10

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

This was backed out for 63. It will need to be fixed in 64, as we are removing bootstrap from Screenshots in 64.

Comment 11

[Pascal Chevrel:pascalc]

(In reply to Jared Hirsch [:_6a68] [:jhirsch] from comment #10)
> This was backed out for 63. It will need to be fixed in 64, as we are
> removing bootstrap from Screenshots in 64.

Updating flags accordingly

Comment 12

[David Durst [:ddurst]]

Moving to Menus because it seems most appropriate. NI adw because of the pageAction relationship. Now that 64 is on Nightly, that patch should be re-landed and if this move is inaccurate, perhaps Drew can help us identify the proper place?

Comment 13

[Drew Willcoxon :adw]

Thanks, we've been putting front-end/browser page action bugs (those involving browser-pageActions.js, PageActions.jsm, and related CSS) in Toolbars and Customization, so I'll move this there, but it's not a big deal

Comment 14

[:Gijs (out for now; he/him)]

P1 because it needs addressing this cycle.

Barry, do you have any cycles to work on this?

Drew, is there a better fix here than adding a bunch of special-casing for screenshots? That feels unfortunate, but I can't really think of something else to do...

Comment 15

[Wil Clouser [:clouserw]]

To be clear:  you want Screemshots to re-land the patch that was backed out?

(totally typo'd that, but I'm leaving it, because it sounds awesome)

Comment 16

[Barry Chen]

I think the patch https://bugzilla.mozilla.org/show_bug.cgi?id=1483085 fixes this.  (I'm not certain though.)

If not, then I can work on it.

Comment 17

[:Gijs (out for now; he/him)]

(In reply to Wil Clouser [:clouserw] from comment #15)
> To be clear:  you want Screemshots to re-land the patch that was backed out?
> 
> (totally typo'd that, but I'm leaving it, because it sounds awesome)

bug 1466575 is marked fixed, and comment 9 says it was backed out 'temporarily', and 64 was marked as tracking and affected, so I was under the impression this had already re-landed. I take it from your comment that it hadn't, but then I don't understand why that bug isn't open.

Now comment #16 seems to suggest the fix will be in a third bug?

So at this point, I would have expected the fix to be in this bug, and bug 1466575 to be open. Given there's already a patch on the other bug, I guess maybe we should dupe this to bug 1483085 and reopen bug 1466575? Either way, it would be nice if the bugs could reflect the state of the tree, one way or another...

(In reply to Barry Chen from comment #16)
> I think the patch https://bugzilla.mozilla.org/show_bug.cgi?id=1483085 fixes
> this.  (I'm not certain though.)
> 
> If not, then I can work on it.

WFM. But if we're going to leave this open, we should probably not have it track a release that doesn't have the bug (so far)...

Comment 18

[Drew Willcoxon :adw]

I don't know why it wouldn't persist across restart, and I don't think bug 1483085 would fix it.  PageActions.jsm persists all actions, regardless of whether they're built in or not.  There shouldn't be any need to special-case screenshots.  But it looks like it currently does persist across restarts?  And on a fresh build, it's not in the urlbar by default.  So it's hard to debug right now if the problem doesn't seem to happen.

Comment 19

[Barry Chen]

It was baffling when I looked into it.  At first it looked like `_purgeUnregisteredPersistedActions` in PageActions.jsm was removing Screenshots's action id.  But my local build eventually started working even though I only added debug statements.

Comment 20

[Barry Chen]

(In reply to Barry Chen from comment #19)
> It was baffling when I looked into it.  At first it looked like
> `_purgeUnregisteredPersistedActions` in PageActions.jsm was removing
> Screenshots's action id.  But my local build eventually started working even
> though I only added debug statements.

I was testing on a build with the patches from Bug 1466575 and Bug 1483085 included.

Comment 21

[:Gijs (out for now; he/him)]

(In reply to Barry Chen from comment #20)
> (In reply to Barry Chen from comment #19)
> > It was baffling when I looked into it.  At first it looked like
> > `_purgeUnregisteredPersistedActions` in PageActions.jsm was removing
> > Screenshots's action id.  But my local build eventually started working even
> > though I only added debug statements.
> 
> I was testing on a build with the patches from Bug 1466575 and Bug 1483085
> included.

Can you clarify the current state of Firefox 63/64, and which of these three bugs should be open/closed, prioritized, or tracking a release, and apply any changes necessary to make the bugs match the current state of the world? This bug keeps coming up in our triage queries, and at the moment I'm really not sure what to do with it because the current state of the bug and what people are saying don't seem to match.

Comment 22

[Barry Chen]

Sorry about that.  :_6a68 will re-land the patch from Bug 1466575.  I've set 64 to unaffected in the meantime.

Comment 23

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Waiting on a Try run to re-land the webextension page action for 64 (bug 1466575). I'll set 64 affected when that lands (assuming I can still reproduce the bug).

I'll run through the other dependencies on bug 1422437 and make sure 64 affected is set for those that are still valid.

Comment 24

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

Hey _6a68, now that bug 1466575 has re-landed, does this bug affect 64 again?

Comment 25

[Darkspirit]

Yes, it is back.

Comment 26

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

If the user has expressed a preference, Firefox really needs to respect it across restarts. Marking P1.

Comment 27

[Drew Willcoxon :adw]

The problem is in the interaction between webextensions page actions and PageActions.jsm.  The webextensions side is calling action.remove() on shutdown, which tells PageActions.jsm to forget about the action.  Other extension-based page actions are affected too (I'm guessing, but I don't see why they wouldn't be).  I swear this used to work.

Comment 28

[Drew Willcoxon :adw]

It's because in ext-pageAction.js, on shutdown the reason is no longer "APP_SHUTDOWN", it's the number 2: https://dxr.mozilla.org/mozilla-central/rev/645939049d7e41e634ab7a315ab99a422db080bf/browser/components/extensions/parent/ext-pageAction.js#128

Comment 29

[Drew Willcoxon :adw]

Hmm, sometimes the reason is 2 and sometimes it's "APP_SHUTDOWN".  For screenshots it's always 2, but for a random extension I installed from AMO, it's "APP_SHUTDOWN" and therefore this bug doesn't happen for it.

Does anyone on the webextensions or screenshots side know why?

Comment 30

[Drew Willcoxon :adw]

Here's the stack when onShutdown in ext-pageAction.js is called for screenshots:

> onShutdown@chrome://browser/content/parent/ext-pageAction.js:129:11
> ExtensionAPI/<@resource://gre/modules/ExtensionCommon.jsm:339:9
> wrapper@resource://gre/modules/ExtensionCommon.jsm:282:14
> emit@resource://gre/modules/ExtensionCommon.jsm:310:24
> emit@resource://gre/modules/Extension.jsm:1434:12
> shutdown@resource://gre/modules/Extension.jsm:1918:5
> async*shutdown@resource://gre/modules/LegacyExtensionsUtils.jsm:220:13
> async*stop@resource://gre/modules/addons/XPIProvider.jsm -> file:///Users/adw/mc/obj-artifact/dist/Nightly.app/Contents/Resources/browser/features/[e-mail]/bootstrap.js:178:26
> shutdown@resource://gre/modules/addons/XPIProvider.jsm -> file:///Users/adw/mc/obj-artifact/dist/Nightly.app/Contents/Resources/browser/features/[e-mail]/bootstrap.js:133:5
> callBootstrapMethod@resource://gre/modules/addons/XPIProvider.jsm:1589:20
> async*_shutdown@resource://gre/modules/addons/XPIProvider.jsm:1725:12
> async*shutdown@resource://gre/modules/addons/XPIProvider.jsm:1718:28
> async*startup/<@resource://gre/modules/addons/XPIProvider.jsm:2192:53
> async*trigger@resource://gre/modules/AsyncShutdown.jsm:710:23
> _wait@resource://gre/modules/AsyncShutdown.jsm:857:7
> wait@resource://gre/modules/AsyncShutdown.jsm:841:28
> observe@resource://gre/modules/AsyncShutdown.jsm:524:17
> goQuitApplication@chrome://global/content/globalOverlay.js:53:3
> oncommand@chrome://browser/content/hiddenWindow.xul:1:1

Extension.shutdown() has a `reason` param, and judging by how it calls updatePermissions(), it looks like `reason` is supposed to be an APP_* string, not a number.  But in this case the caller of Extension.shutdown(), EmbeddedExtension.shutdown(), passes a number.  So I guess that's where the problem is?  I see that Extension.jsm has a mapping called BOOTSTRAP_REASON_TO_STRING_MAP from numbers to strings, so that's probably supposed to be used somewhere along the way?

Comment 31

[Drew Willcoxon :adw]

Attachment: Bug 1483033 - EmbeddedExtension should pass string startup and shutdown reasons to the embedded webextension, not numeric reasons.

Comment 32

[Kris Maglione [:kmag]]

Comment on attachment 9008942 [details]
Bug 1483033 - EmbeddedExtension should pass string startup and shutdown reasons to the embedded webextension, not numeric reasons.

Kris Maglione [:kmag] has approved the revision.

Comment 33

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ce74ddd3192a
EmbeddedExtension should pass string startup and shutdown reasons to the embedded webextension, not numeric reasons. r=kmag

Comment 34

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/03bf654b9641
Backed out changeset ce74ddd3192a for xpc failures on test_ext_legacy_extension_embedding.

Comment 35

[Cosmin Sabou [:CosminS]]

Backed out for xpc failures on test_ext_legacy_extension_embedding.

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=success,testfailed,runnable&searchStr=windows,7,opt,xpcshell,tests,test-windows7-32%2Fopt-xpcshell,x(x)&fromchange=ce74ddd3192adb70e58ab81206184a964842c90d&tochange=03bf654b9641663dbd5816c38cf3deb5562f88ce&selectedJob=199214106

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=199214106&repo=autoland&lineNumber=10032

Backout link: https://hg.mozilla.org/integration/autoland/rev/03bf654b9641663dbd5816c38cf3deb5562f88ce

01:17:14     INFO -  TEST-START | xpcshell-remote.ini:toolkit/components/extensions/test/xpcshell/test_ext_legacy_extension_embedding.js
01:17:14  WARNING -  TEST-UNEXPECTED-FAIL | xpcshell-remote.ini:toolkit/components/extensions/test/xpcshell/test_ext_legacy_extension_embedding.js | xpcshell return code: 0
01:17:14     INFO -  TEST-INFO took 210ms
01:17:14     INFO -  >>>>>>>
01:17:14     INFO -  (xpcshell/head.js) | test MAIN run_test pending (1)
01:17:14     INFO -  (xpcshell/head.js) | test run_next_test 0 pending (2)
01:17:14     INFO -  (xpcshell/head.js) | test MAIN run_test finished (2)
01:17:14     INFO -  running event loop
01:17:14     INFO -  xpcshell-remote.ini:toolkit/components/extensions/test/xpcshell/test_ext_legacy_extension_embedding.js | Starting check_remote
01:17:14     INFO -  (xpcshell/head.js) | test check_remote pending (2)
01:17:14     INFO -  TEST-PASS | xpcshell-remote.ini:toolkit/components/extensions/test/xpcshell/test_ext_legacy_extension_embedding.js | check_remote - [check_remote : 1441] useRemoteWebExtensions matches - true == true

Comment 36

[Drew Willcoxon :adw]

Kris, I don't know this code at all.  Could you please help out?  AddonManagerInternal._getProviderByName(...) is undefined during test_ext_legacy_extension_embedding.js, and I have no idea why or what that means.

> 01:17:14     INFO -  Unexpected exception TypeError: AddonManagerInternal._getProviderByName(...) is undefined; can't access its "BOOTSTRAP_REASONS" property at resource://gre/modules/AddonManager.jsm:2910
> 01:17:14     INFO -  get BOOTSTRAP_REASONS@resource://gre/modules/AddonManager.jsm:2910:12
> 01:17:14     INFO -  @resource://gre/modules/AddonManager.jsm:2807:9
> 01:17:14     INFO -  get@resource://gre/modules/XPCOMUtils.jsm:118:43
> 01:17:14     INFO -  get BOOTSTRAP_REASON_TO_STRING_MAP@resource://gre/modules/AddonManager.jsm:2915:5
> 01:17:14     INFO -  startup/this.startupPromise<@resource://gre/modules/LegacyExtensionsUtils.jsm:195:7
> 01:17:14     INFO -  startup@resource://gre/modules/LegacyExtensionsUtils.jsm:142:27
> 01:17:14     INFO -  test_embedded_webextension_utils@Z:/task_1536885938/build/tests/xpcshell/tests/toolkit/components/extensions/test/xpcshell/test_ext_legacy_extension_embedding.js:75:36
> 01:17:14     INFO -  async*run_next_test/_run_next_test/<@Z:\task_1536885938\build\tests\xpcshell\head.js:1441:22
> 01:17:14     INFO -  async*_run_next_test@Z:\task_1536885938\build\tests\xpcshell\head.js:1441:10
> 01:17:14     INFO -  run@Z:\task_1536885938\build\tests\xpcshell\head.js:692:9
> 01:17:14     INFO -  _do_main@Z:\task_1536885938\build\tests\xpcshell\head.js:219:3
> 01:17:14     INFO -  _execute_test@Z:\task_1536885938\build\tests\xpcshell\head.js:533:5

Comment 37

[h_o_w_]

Comment on attachment 9008942 [details]
Bug 1483033 - EmbeddedExtension should pass string startup and shutdown reasons to the embedded webextension, not numeric reasons.

If I'm reading this right this version applies
> AddonManagerPrivate.BOOTSTRAP_REASON_TO_STRING_MAP[reason]
twice to the same reason variable.

once at https://hg.mozilla.org/integration/autoland/rev/ce74ddd3192a#l2.35
and second time inside the startup() call at https://hg.mozilla.org/integration/autoland/rev/ce74ddd3192a#l1.13

same for shutdown()
https://hg.mozilla.org/integration/autoland/rev/ce74ddd3192a#l2.54
https://hg.mozilla.org/integration/autoland/rev/ce74ddd3192a#l1.53

Comment 38

[Drew Willcoxon :adw]

Thanks.  I debugged this more without the patch applied, and on EmbeddedExtension.shutdown(), sometimes `reason` is 2 and sometimes it's APP_SHUTDOWN.  It's 2 for screenshots, so the patch is right in that case, but it's APP_SHUTDOWN for webcompat, so it's wrong in that case.  Maybe the problem here is in screenshots's bootstrap.js, where it calls `webExtension.shutdown(reason)` -- maybe it should be the one converting the numeric reason to a string reason.

Also, EmbeddedExtension.startup() makes a new Extension and calls startup() on it and passes `reason`, but Extension.startup() doesn't have a `reason` parameter at all.  Instead EmbeddedExtension should be passing `reason` to the Extension constructor.

Comment 39

[Drew Willcoxon :adw]

I'll post some new patches for this.

Comment 40

[Drew Willcoxon :adw]

Attachment: Bug 1483033 - Fix LegacyExtensionsUtils comments and pass startup reason to Extension constructor.

The jsdoc params for `EmbeddedExtension.startup` and `shutdown` don't seem to be right. These methods expect string reasons, not numeric reasons.

Also, when `startup` creates a new `Extension`, it passes only one argument, the add-on data, and when it calls `extension.startup`, it passes a reason, but that's wrong. `Extension`'s constructor expects the reason as its second argument, and `startup` doesn't take any arguments at all.

Comment 41

[Drew Willcoxon :adw]

Attachment: Bug 1483033 - Pass correct startup and shutdown reasons to screenshots's embedded webextension.

`webExtension` in bootstrap.js's `start` and `stop` functions is an `EmbeddedExtension`, and `startupReason` and the `reason` argument passed to `stop` is a numeric reason.  `EmbeddedExtension.startup` and `shutdown` take string reasons, not numeric reasons, contrary to their jsdocs.

Also, we can use `AddonManagerPrivate` instead of hardcoding `APP_STARTUP` and `APP_SHUTDOWN`.

This patch fixes bug 1483033.

Comment 42

[Kris Maglione [:kmag]]

Comment on attachment 9009832 [details]
Bug 1483033 - Fix LegacyExtensionsUtils comments and pass startup reason to Extension constructor.

Kris Maglione [:kmag] has approved the revision.

Comment 43

[Drew Willcoxon :adw]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b08de562f66dc0dbb13c636ad460df83bc0cf0b3

Comment 45

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Comment on attachment 9009833 [details]
Bug 1483033 - Pass correct startup and shutdown reasons to screenshots's embedded webextension.

Jared Hirsch [:_6a68] [:jhirsch] has approved the revision.

Comment 46

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bc8e00559118
Fix LegacyExtensionsUtils comments and pass startup reason to Extension constructor. r=kmag
https://hg.mozilla.org/integration/autoland/rev/6c0f7c054498
Pass correct startup and shutdown reasons to screenshots's embedded webextension. r=_6a68

Comment 47

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/bc8e00559118
https://hg.mozilla.org/mozilla-central/rev/6c0f7c054498

Comment 48

[Mohammad Maruf Rahman]

I have reproduced  this bug with Nightly 62.0a1 (2018-08-13) on Windows 7, 64 Bit!
This bug's fix is verified with latest Nightly!

Build ID 	20180921100113
User Agent 	Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:64.0) Gecko/20100101 Firefox/64.0

Comment 49

[Darkspirit]

Nightly 64 x64 20180921100113 de_DE @ Debian Testing (KDE, Xorg, GTX1060)
Yes, icon removal persists now, but the screenshot icon is still shown by default in a new profile and in my existing profile.
Is that intended?

Comment 50

[Drew Willcoxon :adw]

(In reply to Jan Andre Ikenmeyer [:darkspirit] from comment #49)
> Yes, icon removal persists now, but the screenshot icon is still shown by
> default in a new profile and in my existing profile.
> Is that intended?

jgruen, is screenshots supposed to appear in the urlbar by default now?  It does, but it didn't before the recent webextension conversion bugs, so it seems like an accident.

The reason it does is that all webextension page actions start out pinned to the urlbar by default [1], and screenshots specifies `"show_matches": ["<all_urls>"]` [2], so it's enabled for every page.  Both those things means it shows up by default.

[1] https://dxr.mozilla.org/mozilla-central/rev/ebeba937ca2a01df86d98089f2368bb975182dcc/browser/components/extensions/parent/ext-pageAction.js#90
[2] https://dxr.mozilla.org/mozilla-central/rev/ebeba937ca2a01df86d98089f2368bb975182dcc/browser/extensions/screenshots/webextension/manifest.json#39

Comment 51

[John Gruen [:jgruen]]

Sorry for the late reply here. It should not. Should we file a new issue or track that here?

Comment 52

[Drew Willcoxon :adw]

Thanks, I filed bug 1494135.