Closed Bug 1443749 Opened 2 years ago Closed 2 years ago

extension sidebars are reloaded unecessarily

Categories

(WebExtensions :: General, defect, P1)

x86_64
All
defect

Tracking

(firefox-esr52 unaffected, firefox58 unaffected, firefox59 unaffected, firefox60 verified, firefox61 verified)

VERIFIED FIXED
mozilla60
Tracking Status
firefox-esr52 --- unaffected
firefox58 --- unaffected
firefox59 --- unaffected
firefox60 --- verified
firefox61 --- verified

People

(Reporter: cbadescu, Assigned: mixedpuppy)

References

Details

(Keywords: regression)

Attachments

(1 file)

[Affected versions]:
- Nightly 60.0a1

[Affected Platforms]:
- All Mac
- All Linux

[Prerequisites]:
- Have a Firefox profile with the latest version of the "Firefox Notes" add-on installed from the Test Pilot experiments page: https://testpilot.firefox.com/experiments/notes.

[Steps to reproduce]:
1. Open the browser with the profile from prerequisites and navigate to "https://en.wikipedia.org/wiki/Tree" page.
2. Select a text and right click on it.
3. Click the "Send to Notes" option and observe the behavior.

[Expected result]:
- The text is instantly sent to the Notes sidebar.

[Actual result]:
- The sidebar becomes blank for a second and the text is not sent into the sidebar.

[Regression]:
- I've managed to find a regression window using the Mozregression tool. Here are the results:
Last good revision: 7433031c9274b8299c3e2d50613de74352684bc1
First bad revision: 95a6bd5b90452da6eee11eb8ee53ef80c91565c6
Pushlog: https://goo.gl/3o4Et5

From the pushlog it seems that bug 1398713 might have caused this.
Shane, can you please take a look at this issue?

[Notes]:
- This issue is reproducible even if you are logged in a Firefox Notes account.
- This issue is not reproducible on Windows 10 x64.
- I am not authorized to see the bug from the pushlog.
- I've obtain the same pushlog results for two others issues: https://github.com/mozilla/notes/issues/740 and https://github.com/mozilla/notes/issues/742.
- Here is a screen recording of the issue: https://goo.gl/ipsKpF
Flags: needinfo?(mixedpuppy)
This is indeed due to bug 1398713.  The fast way to address this is to fix notes.  The slow way to address it is to land a fix in fx61 unless someone wants to approve a beta uplift.

ISTM that notes is calling sidebarAction.open all the time.  You should first check if it is open by calling sidebarAction.isOpen.  I don't know enough about notes code to make any more detailed suggestion.
Flags: needinfo?(mixedpuppy) → needinfo?(cristina.badescu)
(In reply to Shane Caraveo (:mixedpuppy) from comment #1)
> This is indeed due to bug 1398713.  The fast way to address this is to fix
> notes.  The slow way to address it is to land a fix in fx61 unless someone
> wants to approve a beta uplift.
> 
> ISTM that notes is calling sidebarAction.open all the time.  You should
> first check if it is open by calling sidebarAction.isOpen.  I don't know
> enough about notes code to make any more detailed suggestion.

We will do that, we shall patch Notes and fix it up! Thank you for your suggestion
Flags: needinfo?(cristina.badescu)
Assignee: nobody → mixedpuppy
Blocks: 1398713
Keywords: regression
Priority: -- → P1
Summary: The Firefox Notes context menu option is no longer working on Linux and Mac Os'es → extension sidebars are reloaded unecessarily
Just some notes from https://gist.github.com/vladikoff/808c1c6477f0090840e54144c23b803e

I tried to go with the `isOpen` route and fix this in the Notes extension but got hit with the issues of `Error: sidebarAction.open may only be called from a user input handler` for both `browser.browserAction` and `browser.contextMenus`. It seems this is because `browser.sidebarAction.open();` cannot be called from within the async context of `isOpen`.
Attachment #8957691 - Flags: review?(kmaglione+bmo)
Comment on attachment 8957691 [details]
Bug 1443749 only reload sidebar when the url has changed

https://reviewboard.mozilla.org/r/226610/#review232462

::: browser/base/content/browser-sidebar.js:411
(Diff revision 1)
>    _show(commandID) {
> -    return new Promise((resolve, reject) => {
> +    return new Promise(async (resolve, reject) => {

Please just make `show` an async function, and try wrapping just the `addEventListener` bit in `new Promise` if you can.

::: browser/base/content/webext-panels.js:96
(Diff revision 1)
>  
>  async function loadPanel(extensionId, extensionUrl, browserStyle) {
> +  let browser = document.getElementById("webext-panels-browser");
> +  if (browser) {
> +    if (browser.currentURI.spec === extensionUrl) {
> +      return Promise.resolve();

Just `return;` will do. Async functions always return a promise.

::: browser/base/content/webext-panels.js:114
(Diff revision 1)
> +  // Don't force a reload if the url has not changed.
> +  let readyPromise = new Promise(resolve => {
> +    browser.addEventListener("load", event => {
> +      // We're handling the 'load' event before it bubbles up to the usual
> +      // (non-capturing) event handlers. Let it bubble up before resolving.
> +      setTimeout(resolve, 0);

`Services.tm.dispatchToMainThread`
Comment on attachment 8957691 [details]
Bug 1443749 only reload sidebar when the url has changed

https://reviewboard.mozilla.org/r/226610/#review232468
Attachment #8957691 - Flags: review?(kmaglione+bmo) → review+
Pushed by mixedpuppy@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/3bf5123d0218
only reload sidebar when the url has changed r=kmag
https://hg.mozilla.org/mozilla-central/rev/3bf5123d0218
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
the bug causing this regression did not land in 59.
Depends on: 1445080
This issue is no longer reproducible on latest Nightly build 61.0a1 (Build ID: 20180325220121) and latest Firefox Beta 60.0b6 (Build ID: 20180322152034), on Windows 10 x64, Mac 10.12.6 and Arch Linux 4.12. Marking this as Verified Fixed.
Status: RESOLVED → VERIFIED
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.