Closed Bug 1483033 Opened 2 years ago Closed 1 year ago

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

Categories

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

defect

Tracking

()

RESOLVED FIXED
Firefox 64
Tracking Status
firefox-esr52 --- unaffected
firefox-esr60 --- unaffected
firefox61 --- unaffected
firefox62 --- unaffected
firefox63 - unaffected
firefox64 + fixed

People

(Reporter: darkspirit, Assigned: adw)

References

Details

(Keywords: nightly-community, regression)

Attachments

(3 files, 1 obsolete file)

Attached video 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.
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.
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.
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.
Depends on: 1483572
Duplicate of this bug: 1483571
Blocks: 1483591
Duplicate of this bug: 1482697
[Tracking Requested - why for this release]:
Screenshot icon always comes back to the address bar even if removed
Tracked for 63 since it's a recent regression.
Ian, could you help finding an owner for this regression? Thanks
Flags: needinfo?(ianb)
(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.)
This was backed out for 63. It will need to be fixed in 64, as we are removing bootstrap from Screenshots in 64.
Flags: needinfo?(ianb)
(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
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?
Component: Screenshots → Menus
Flags: needinfo?(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
Component: Menus → Toolbars and Customization
Flags: needinfo?(adw)
OS: Linux → Unspecified
Hardware: x86_64 → Unspecified
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...
Flags: needinfo?(bchen)
Flags: needinfo?(adw)
Priority: -- → P1
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)
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.
Flags: needinfo?(bchen)
(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)...
Priority: P1 → --
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.
Flags: needinfo?(adw)
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.
(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.
(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.
Flags: needinfo?(bchen)
Sorry about that.  :_6a68 will re-land the patch from Bug 1466575.  I've set 64 to unaffected in the meantime.
Flags: needinfo?(bchen) → needinfo?(jhirsch)
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.
Flags: needinfo?(jhirsch)
Hey _6a68, now that bug 1466575 has re-landed, does this bug affect 64 again?
Flags: needinfo?(jhirsch)
If the user has expressed a preference, Firefox really needs to respect it across restarts. Marking P1.
Priority: -- → P1
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.
Assignee: nobody → adw
Status: NEW → ASSIGNED
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
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?
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/screenshots@mozilla.org/bootstrap.js:178:26
> shutdown@resource://gre/modules/addons/XPIProvider.jsm -> file:///Users/adw/mc/obj-artifact/dist/Nightly.app/Contents/Resources/browser/features/screenshots@mozilla.org/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 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.
Attachment #9008942 - Flags: review+
Pushed by dwillcoxon@mozilla.com:
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
Backout by csabou@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/03bf654b9641
Backed out changeset ce74ddd3192a for xpc failures on test_ext_legacy_extension_embedding.
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
Flags: needinfo?(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
Flags: needinfo?(kmaglione+bmo)
Flags: needinfo?(jhirsch)
Flags: needinfo?(adw)
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
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.
I'll post some new patches for this.
Flags: needinfo?(kmaglione+bmo)
Attachment #9008942 - Attachment is obsolete: true
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.
`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 on attachment 9009832 [details]
Bug 1483033 - Fix LegacyExtensionsUtils comments and pass startup reason to Extension constructor.

Kris Maglione [:kmag] has approved the revision.
Attachment #9009832 - Flags: review+
Duplicate of this bug: 1492321
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.
Attachment #9009833 - Flags: review+
Pushed by dwillcoxon@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/bc8e00559118
https://hg.mozilla.org/mozilla-central/rev/6c0f7c054498
Status: ASSIGNED → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 64
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
QA Whiteboard: [bugday-20180919]
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?
(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
Flags: needinfo?(jgruen)
Sorry for the late reply here. It should not. Should we file a new issue or track that here?
Flags: needinfo?(jgruen) → needinfo?(adw)
See Also: → 1494135
Thanks, I filed bug 1494135.
Flags: needinfo?(adw)
You need to log in before you can comment on or make changes to this bug.