1483684 - setPopup regressed in 62, breaking react-devtools's popup

Status: VERIFIED FIXED

(WebExtensions :: General, defect, P1)

Description

[:Harald Kirschner :digitarald]

STR:
- Install https://addons.mozilla.org/en-US/firefox/addon/react-devtools/
- Open https://reactjs.org/
- Click the react toolbar icon

ER: Toolbar icon changes and "react detected" popup is shown on click
AR: Toolbar icon changes but the popup shows the no-react-detected version.

This works in 61, but fails in 62 DevEdition and 63 Nightly.

Filed also in react-devtools repo: https://github.com/facebook/react-devtools/issues/1082

This is where setPopup is called:
https://github.com/facebook/react-devtools/blob/83adb171e700f43846f559f3e009acf99d6bba31/shells/webextension/src/background.js#L83

Some debugging shows that getPopup returns the correct new URL which does not seem to be applied though. Tested also without a `default_popup` set, which leads to never seeing any popup.

Comment 1

[Ian Moody [:Kwan] (UTC+0)]

mozregression -g 61 -b 62 -e react_developer_tools-3.2.3-an+fx.xpi -a https://reactjs.org/
Gives me:
Pushlog:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=d6f131d9d8a00d576330ae7fee1ab662d6359574&tochange=331be24ce5e6fecb336729ba857cd2ffd5573970

So bug 1419893 looks like the culprit.

Comment 2

[Oriol Brufau [:Oriol]]

Argh, there are some getProperty calls that I forgot to update when I changed that function. I'm surprised the tests didn't complain.

Comment 3

[Ian Moody [:Kwan] (UTC+0)]

[Tracking Requested - why for this release]:
Breaks web extension API.  Affected extensions include React DevTools, used to debug the popular web framework React.

Comment 4

[Ian Moody [:Kwan] (UTC+0)]

(Hmm, it didn't give me the tracking form.  Maybe because of component?)

Anyway, the why:
Broken part of the web extensions API.
Affected extensions include developer-focused React Devtools for debugging the widely-used React framework, leading to a confusing UX.

Comment 5

[Oriol Brufau [:Oriol]]

Attachment: Bug 1483684 - Fix browserAction ignoring tab-specific or window-specific popups and showing global one instead. r=mixedpuppy

Comment 6

[sami.vanttinen]

I can reproduce exactly the same bug with a browser extension I'm developing. Like reported before, Firefox 61 and earlier are unaffected.

Comment 7

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 9001465 [details]
Bug 1483684 - Fix browserAction ignoring tab-specific or window-specific popups and showing global one instead. r=mixedpuppy

Shane Caraveo (:mixedpuppy) has approved the revision.

Comment 8

[Shane Caraveo (:mixedpuppy)]

Oriol, please request beta uplift on this.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0e1d62e81cb1
Fix browserAction ignoring tab-specific or window-specific popups and showing global one instead. r=mixedpuppy

Comment 10

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/0e1d62e81cb1

Comment 11

[Vlad Jiman]

Verified using FF 62 and Nightly running under Windows 10 x64, the issue was not reproducible on the latest build of Nightly 63 (08/17/2018), it did reproduce on 62.

Comment 12

[Vlad Jiman]

Attachment: 62.png

Comment 13

[Vlad Jiman]

Attachment: Latest Nightly.png

Comment 14

[sami.vanttinen]

The latest Nightly seems to fix the issue for me as well.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Please nominate this for Beta approval when you get a chance.

Comment 16

[Oriol Brufau [:Oriol]]

Attachment: beta.patch

The patch for mozilla-central doesn't apply cleanly to beta. I attach the patch for beta with the merge conflicts fixed.

Approval Request Comment
[Feature/Bug causing the regression]: bug 1419893
[User impact if declined]: clicking a browserAction to open its popup will ignore tab-specific and window-specific popup URLs and will load the globally defined one instead.
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: yes (comment 11 and comment 14)
[Needs manual test from QE? If yes, steps to reproduce]: see comment 0
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: not risky
[Why is the change risky/not risky?]: in bug 1419893 I changed the signature of the getProperty function without realizing some calls were still using the previous one. This patch restores the original signature of getProperty and adds a getPropertyFromDetails function with the new signature.
[String changes made/needed]: none

Comment 17

[Liz Henry (:lizzard)]

Comment on attachment 9001977 [details] [diff] [review]
beta.patch

Fix for a new regression in 62, has verification from users in nightly, adds tests. Let's uplift for beta 19.

Comment 18

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/releases/mozilla-beta/rev/dd14588873be

Comment 19

[Victor Carciu]

Verified as fixed in Firefox 62.0b19 (20180820213041)
I will attach a postfix video.

Comment 20

[Victor Carciu]

Attachment: Postfix Video FF62