Screenshots page action throws error if clicked twice in a row

RESOLVED FIXED in Firefox 57

Status

defect
RESOLVED FIXED
2 years ago
10 months ago

People

(Reporter: _6a68, Assigned: kmag)

Tracking

unspecified
mozilla58
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox56 unaffected, firefox57 fixed, firefox58 verified)

Details

Attachments

(1 attachment)

Summary: a user-visible exception is thrown by Screenshots in 57 if you click the page action twice in a row.

STR:

1. Open Nightly
2. Load example.com
3. Click the screenshots page action in the page action menu
4. Dismiss the screenshots tour
5. Click the screenshots page action 3 more times (dismiss the overlay, show it again, dismiss it again)

Expected: the screenshots overlay should be removed

Actual: an uncaught exception is rethrown by Screenshots

-----

I bisected Nightly to the following pushlog: 

https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=5a63d8457a2a69a2ad54a50765bff412580df2a4&tochange=14db7c0bcf9ae86c9ec2cc9d3c249a42e459a2a9

-----

Note that screenshots is tracking this bug at https://github.com/mozilla-services/screenshots/issues/3552
This issue is a blocker for Screenshots for 57
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Assignee: nobody → kmaglione+bmo

Comment 3

2 years ago
mozreview-review
Comment on attachment 8912433 [details]
Bug 1403369: Correctly handle content-side errors in tabs.executeScript().

https://reviewboard.mozilla.org/r/183762/#review189272

This is good as is, but can you please check if it also covers bug 1403505, perhaps one less uplift if we need to do that?

Also, I'm guessing there is almost no way to write a reliable test for this.. :/

::: toolkit/components/extensions/ExtensionContent.jsm:265
(Diff revision 1)
>      return this.matcher.matchesWindow(window);
>    }
>  
>    async injectInto(window) {
>      let context = this.extension.getContext(window);
> -
> +    try {

Is this just that the window might be null by the time we get to injecting?  That's probably also bug 1403505 then.

::: toolkit/components/extensions/ExtensionContent.jsm:736
(Diff revision 1)
>        return null;
>      };
>  
> -    let promises = Array.from(this.enumerateWindows(global.docShell), executeInWin)
> +    let promises;
> +    try {
> +      promises = Array.from(this.enumerateWindows(global.docShell), executeInWin)

Not sure what else can throw if the answer to below question is "no".

::: toolkit/components/extensions/ExtensionContent.jsm:786
(Diff revision 1)
>    * enumerateWindows(docShell) {
>      let enum_ = docShell.getDocShellEnumerator(docShell.typeContent,
>                                                 docShell.ENUMERATE_FORWARDS);
>  
>      for (let docShell of XPCOMUtils.IterSimpleEnumerator(enum_, Ci.nsIInterfaceRequestor)) {
> +      try {

I don't know how this stuff works, can the casting to `nsIInterfaceRequestor` in the iterator helper throw in the same/similar case?
Attachment #8912433 - Flags: review?(tomica) → review+
(Assignee)

Comment 4

2 years ago
mozreview-review-reply
Comment on attachment 8912433 [details]
Bug 1403369: Correctly handle content-side errors in tabs.executeScript().

https://reviewboard.mozilla.org/r/183762/#review189272

I'm not concerned about bug 1403505. That error happens when a context is destroyed while we're waiting to execute its content scripts. It can only ever happen during extension shutdown, and is pretty unlikely even then.

And yes, I couldn't think of a good way to test this. It depends on a race, and even if I could find a way to reliably trigger that race, the conditions that trigger it would almost certainly change.

> Is this just that the window might be null by the time we get to injecting?  That's probably also bug 1403505 then.

No, it's to catch any sort of unexpected error that might happen. Bug 1403505 doesn't have anything to do with the window being null. That causes different errors :)

> Not sure what else can throw if the answer to below question is "no".

*Anything* could throw. The pont of this is to catch unexpected errors.

> I don't know how this stuff works, can the casting to `nsIInterfaceRequestor` in the iterator helper throw in the same/similar case?

Nope.
(Assignee)

Comment 5

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/557dd09e0e29b0ac8f8e9e67a218e4b444edf6b4
Bug 1403369: Correctly handle content-side errors in tabs.executeScript(). r=zombie

Comment 6

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/557dd09e0e29
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
I have retested this issue on latest Nightly (58.0a1, Build ID: 20171003100226)(x64) and the issue is no longer reproducible.
I can confirm that the issue was reproducible on older Nightly builds (eg: build from 2017-09-21).

Verified as fixed on Windows 10 x64, Windows 7 x64, Mac OS 10.12 and Ubuntu 14.04 x64 using latest Nightly (58.0a1) build.
Do we want to uplift that to 57 as it seems affected too?
Flags: needinfo?(kmaglione+bmo)
This fix is important to the Screenshots experience, so we'd like to see it uplifted.
(Assignee)

Comment 10

2 years ago
Comment on attachment 8912433 [details]
Bug 1403369: Correctly handle content-side errors in tabs.executeScript().

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1379148
[User impact if declined]: This causes obscure failures in the Screenshots system add-on, and potentially other add-ons too.
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: N/A
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: No.
[Why is the change risky/not risky?]: It simply adds some additional error checks to handle certain non-fatal errors more gracefully.
[String changes made/needed]: None.
Flags: needinfo?(kmaglione+bmo)
Attachment #8912433 - Flags: approval-mozilla-beta?

Comment 11

2 years ago
With screenshots going to 100% of users in 56, this seems like a low risk, high impact fix. Noting it doesn't cause a known crash or security issue though.
Comment on attachment 8912433 [details]
Bug 1403369: Correctly handle content-side errors in tabs.executeScript().

Recent problem that makes using Screenshots difficult, Beta57+
Attachment #8912433 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(In reply to Kris Maglione [:kmag] (long backlog; ping on IRC if you're blocked) from comment #10)
> [Is this code covered by automated tests?]: Yes
> [Has the fix been verified in Nightly?]: Yes
> [Needs manual test from QE? If yes, steps to reproduce]: N/A

Setting qe-verify- based on Kris's assessment on manual testing needs and the fact that this fix has automated coverage.
Flags: qe-verify-

Updated

10 months ago
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.