Closed Bug 1437178 Opened 2 years ago Closed 2 years ago

pageAction visibility issues when using show_matches and hide_matches

Categories

(WebExtensions :: General, defect, P2)

59 Branch
defect

Tracking

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

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

People

(Reporter: Oriol, Assigned: Oriol)

References

Details

Attachments

(3 files, 2 obsolete files)

Attached file test.xpi (obsolete) —
Load attached webextension

The manifest has
  "page_action": {
    "show_matches": ["<all_urls>"],
    "hide_matches": ["<all_urls>"]
  }

hide_matches takes precedence over show_matches, so the pageAction should be hidden by default, but it's shown.

https://searchfox.org/mozilla-central/rev/7ed2a637dc305e90fed6ffb041c6b8fa162b66bd/browser/components/extensions/ext-pageAction.js#44
Comment on attachment 8949867 [details]
Bug 1437178 - Fix various pageAction visibiltiy issues when using show_matches and hide_matches

https://reviewboard.mozilla.org/r/219186/#review224966

Well, that doesn't really make sense to do anyway.  I'm curious if/how page action is usable in this situation.  I'm guessing show still works?
Attachment #8949867 - Flags: review?(mixedpuppy) → review+
(In reply to Shane Caraveo (:mixedpuppy) from comment #2)
> Well, that doesn't really make sense to do anyway.  I'm curious if/how page
> action is usable in this situation.  I'm guessing show still works?

show_matches and hide_matches only determine the default shownness. show and hide are tab-specific, so they have more precedence.
Assignee: nobody → oriol-bugzilla
Status: NEW → ASSIGNED
Keywords: checkin-needed
Argh, in fact the problem is much worse, e.g. with

  "page_action": {
    "show_matches": ["<all_urls>"],
    "hide_matches": ["*://example.com/*"]
  }

if I load example.com, the pageAction is shown. It's fixed when you switch to another tab and then back to the example.com tab. That's why the test didn't fail. In fact the test that I added seems useless because it also passes without the changes.

Will think something else.
Keywords: checkin-needed
So I was thinking something like

    let show, showMatches, hideMatches;
    if (!show_matches.length || hide_matches.includes("<all_urls>")) {
      // Always hide by default
      show = false;
    } else if (show_matches.includes("<all_urls>") && !hide_matches.length) {
      // Always show by default
      show = true;
    } else {
      // Might show or hide depending on the URL
      showMatches = new MatchPatternSet(show_matches);
      hideMatches = new MatchPatternSet(hide_matches);
    }

And then in updateButton

    if (show == null) {
      // Set show via pattern matching.
      let uri = tab.linkedBrowser.currentURI;
      show = tabData.showMatches.matches(uri) && !tabData.hideMatches.matches(uri);
    }
    this.browserPageAction.setDisabled(!show, window);

But, <all_urls> is not supposed to match things like about:debugging, right?

The above has the problem that an add-on with

  "page_action": {
    "show_matches": ["<all_urls>"]
  },

shows the pageAction everywhere, including about:debugging, but an add-on with

  "page_action": {
    "show_matches": ["<all_urls>"],
    "hide_matches": ["*://example.com/*"]
  },

does not show it in about:debugging. Currently it's shown in both cases.

For consistency, even if show_matches contains <all_urls> and hide_matches is empty, pattern matching can't be avoided.

But the case
  !show_matches.length || hide_matches.includes("<all_urls>")

can still be optimized, which is good because I expect !show_matches.length to happen frequently.
Currently it is not optimized! https://searchfox.org/mozilla-central/rev/7ed2a637dc305e90fed6ffb041c6b8fa162b66bd/browser/components/extensions/ext-pageAction.js#241

In fact it could be generalized to "every pattern in show_matches is superseded by or equal to some pattern in hide_matches", but not sure if it's worth checking because it would be pointless for an extension author to do that.
Oh, then isShown returns undefined for tabs where we haven't evaluated yet whether the pageAction should be shown or not. Could be a feature, but looks like a bug.
Comment on attachment 8949867 [details]
Bug 1437178 - Fix various pageAction visibiltiy issues when using show_matches and hide_matches

https://reviewboard.mozilla.org/r/219184/#review225048

::: browser/components/extensions/ext-browser.js
(Diff revision 3)
> -    if (!(~stateFlags & (flags.STATE_IS_WINDOW | flags.STATE_START) ||
> -          this.lastLocation.has(browser))) {
> -      this.lastLocation.set(browser, request.URI);
> -    }
> -  }
> -

I remove this code because it updates the last location of the browser, then onLocationChange does not see the change and does not emit the "location-change" event, so the page action is not updated.
Comment on attachment 8949867 [details]
Bug 1437178 - Fix various pageAction visibiltiy issues when using show_matches and hide_matches

I think with this it's completely fixed. I ran the updated test without the other changes, and there were 74 failures out of 248 checks. The current firefox behavior is so buggy:

 - If show_matches contains <all_urls>, the page action is always shown by default
   - This includes some about: pages, which are not matched by <all_urls>
   - This includes pages that match some hide_matches (but this is fixed when switching tabs)
 - Page actions are not shown in the active tabs when the extension is installed, even if they match some show_matches (if show_matches does not contain <all_urls>)
 - isShown does not return the right value in various cases
 - Some location-change events are not detected, so the page action may not be updated when navigating
Attachment #8949867 - Flags: review+ → review?(mixedpuppy)
Summary: Webext with both show_matches and hide_matches set to <all_urls> should be hidden by default → pageAction visibiltiy issues when using show_matches and hide_matches
Comment on attachment 8949867 [details]
Bug 1437178 - Fix various pageAction visibiltiy issues when using show_matches and hide_matches

https://reviewboard.mozilla.org/r/219186/#review225234

::: browser/components/extensions/ext-browser.js
(Diff revision 3)
>        this.emit("tab-select", nativeTab);
>        this.emit("location-change", nativeTab);
>      }
>    }
>  
> -  onStateChange(browser, webProgress, request, stateFlags, statusCode) {

This also affects browser and sidebar actions.  I need to think about that more, but the other issues below need to be addressed.

::: browser/components/extensions/ext-pageAction.js:47
(Diff revision 3)
>  
> -    // If <all_urls> is present, the default is to show the page action.
> -    let show = !!options.show_matches && options.show_matches.includes("<all_urls>");
> -    let showMatches = new MatchPatternSet(options.show_matches || []);
> -    let hideMatches = new MatchPatternSet(options.hide_matches || []);
> +    let show_matches = options.show_matches || [];
> +    let hide_matches = options.hide_matches || [];
> +
> +    let show, showMatches, hideMatches;
> +    if (!show_matches.length || hide_matches.includes("<all_urls>")) {

Show/hideMatches is now undefined, yet new code you added doesn't check for that.  I'm inclined to think that they should always be set as before.

showing pageActions should always default to false unless show_matches overrides that with all_urls.  Now the "show" value can only be either undefined or false, thus this doesn't really make sense.

I'm inclined towards just removing "show" and always using the match patterns.

::: browser/components/extensions/ext-pageAction.js:107
(Diff revision 3)
>          },
>        }));
> +
> +      // If the page action is only enabled in some URLs, check if it should
> +      // be shown in active tabs.
> +      if (show == null) {

use "===".  using == here was confusing for a moment, because there is no way show could be null.  This code should also be unecessary if show === false.

::: browser/components/extensions/ext-pageAction.js:165
(Diff revision 3)
> -    let tabData = this.tabContext.get(window.gBrowser.selectedTab);
> +    let tab = window.gBrowser.selectedTab;
> +    let tabData = this.tabContext.get(tab);
>      let title = tabData.title || this.extension.name;
>      this.browserPageAction.setTitle(title, window);
>      this.browserPageAction.setTooltip(title, window);
> -    this.browserPageAction.setDisabled(!tabData.show, window);
> +    let show = this.isShown(tab, tabData);

This now does the matching for every update, I'd like to make sure this isn't called unecessarily.  That was the point of doing this in handleLocationChange.
Attachment #8949867 - Flags: review?(mixedpuppy) → review-
Comment on attachment 8949867 [details]
Bug 1437178 - Fix various pageAction visibiltiy issues when using show_matches and hide_matches

https://reviewboard.mozilla.org/r/219186/#review225234

> use "===".  using == here was confusing for a moment, because there is no way show could be null.  This code should also be unecessary if show === false.

i.e. use if (show !== false) instead to make this clear what it is doing.
(In reply to Shane Caraveo (:mixedpuppy) from comment #11)
> Show/hideMatches is now undefined, yet new code you added doesn't check for
> that.

Yes, I do. I use show==null

> I'm inclined to think that they should always be set as before.
> showing pageActions should always default to false unless show_matches
> overrides that with all_urls.  Now the "show" value can only be either
> undefined or false, thus this doesn't really make sense.
> I'm inclined towards just removing "show" and always using the match
> patterns.

That's unnecessary. The common case is no show_matches and no hide_matches.
Then match patterns will never match, so that can be optimized out by setting
the default show to false.

So there are three states for show:

 - false. This means the page action is not shown.
   This happens by default if there is no show_matches or if hide_matches has <all_urls>.
   It can also be set in a tab via pageAction.hide(tabId), e.g. to override show_matches.
 - true. This means the page action is shown.
   This will never happen by default because <all_urls> does not really match all urls,
   but can be set in a tab via pageAction.show(tabId).
 - undefined. This means the page action may be shown or not depending on the tab URL.
   This is the default value when there are some show_matches and hide_matches does not contain <all_urls>.
   Currently can't be set in a tab.

We can (and should!) entirely skip pattern matching in the first two cases.
Further optimization: if show is undefined, the result of pattern matching is cached as a tab-specific value so that pattern matching does not happen again in that tab.

> use "===".  using == here was confusing for a moment, because there is no
> way show could be null.  This code should also be unecessary if show ===
> false.

But ===null does not detect undefined. Do you prefer ===undefined? Or should I use null instead of undefined?

> This now does the matching for every update, I'd like to make sure this
> isn't called unecessarily.  That was the point of doing this in
> handleLocationChange.

No, just once, because the result is cached in my patch. Currently the result is not cached properly, and each time you switch tabs, pattern matching is performed: https://searchfox.org/mozilla-central/rev/7ed2a637dc305e90fed6ffb041c6b8fa162b66bd/browser/components/extensions/ext-pageAction.js#241
Do you prefer it like this? I moved the pattern matching part outside of updateButton and I explicitly set show=null instead of undefined.
(In reply to Oriol Brufau [:Oriol] from comment #13)
> (In reply to Shane Caraveo (:mixedpuppy) from comment #11)

> > use "===".  using == here was confusing for a moment, because there is no
> > way show could be null.  This code should also be unecessary if show ===
> > false.
> 
> But ===null does not detect undefined. Do you prefer ===undefined? Or should
> I use null instead of undefined?

My earlier confusion as I said was entirely around this.  Readability of the code is better if explicit.  However, I think there are things that can be changed, I'll note those in rb.
Attached file test2.xpi (obsolete) —
(In reply to Shane Caraveo (:mixedpuppy) from comment #11)
> Comment on attachment 8949867 [details]
> Bug 1437178 - Fix various pageAction visibiltiy issues when using
> show_matches and hide_matches
> 
> https://reviewboard.mozilla.org/r/219186/#review225234
> 
> ::: browser/components/extensions/ext-browser.js
> (Diff revision 3)
> >        this.emit("tab-select", nativeTab);
> >        this.emit("location-change", nativeTab);
> >      }
> >    }
> >  
> > -  onStateChange(browser, webProgress, request, stateFlags, statusCode) {
> 
> This also affects browser and sidebar actions.  I need to think about that
> more, but the other issues below need to be addressed.

Only browserAction and pageAction use "location-change" event: https://searchfox.org/mozilla-central/search?q=%22location-change%22&case=false&regexp=false&path=

And effectively browserAction is also affected:

1. Extract the attached test2.xpi
2. Load it in about:debugging
3. Open a new tab
4. Click the browserAction, this will set a tab-specific badge text
5. Click some link in the new tab page. This is a navigation, but the badge is not cleared!
6. You can then navigate again, and this time it will be cleared as expected.

So removing this code is good.
Comment on attachment 8949867 [details]
Bug 1437178 - Fix various pageAction visibiltiy issues when using show_matches and hide_matches

https://reviewboard.mozilla.org/r/219186/#review225330

::: browser/components/extensions/ext-browser.js
(Diff revision 4)
>        this.emit("tab-select", nativeTab);
>        this.emit("location-change", nativeTab);
>      }
>    }
>  
> -  onStateChange(browser, webProgress, request, stateFlags, statusCode) {

I still want to know why onStateChange was necessary in the first place so we can be certain of its removal.

::: browser/components/extensions/ext-pageAction.js:57
(Diff revision 4)
> +    //   This is the default value when there are some show_matches and hide_matches
> +    //   doesn't contain <all_urls>. Can't be set in a tab.
> +    let show, showMatches, hideMatches;
> +    let show_matches = options.show_matches || [];
> +    let hide_matches = options.hide_matches || [];
> +    if (!show_matches.length || hide_matches.includes("<all_urls>")) {

If there is no "show_matches" then "hide_matches" is basically a no-op.  Since hide_matches overrides, setting that to all_urls also creates (what should be) a no-op, but in a confusing manor.

The current situation is:

show_matches: ["http://example.com"],
hide_matches: ["<all_urls>"]

will never result in the action being shown on example.com, but clearly the intention is that the action is shown on example.com.  Yes, the author should not have added hide_matches, but the point is that we should always honor show_matches in this case.

IMO Just remove all_urls from hide_matches (logging a warning is fine).  As well, if there are no show_matches, then hide_matches shouldn't have any effect.

show_matches: ["http://example.com/*"],
hide_matches: ["<all_urls>"]

should be the same has having:

show_matches: ["http://example.com/*"],
hide_matches: [] (or no hide_matches in manifest)

Having:

show_matches: [] (or no show_matches in manifest)
hide_matches: [...anything]

is a no-op, and we should probably warn or error out.


let show, showMatches, hideMatches;
if (options.show_matches.length) {
  let hide_matches = options.hide_matches || [];
  if (hide_matches.includes("<all_urls>") {
    hide_matches.delete("<all_urls>")
  }
  showMatches = new MatchPatternSet(options.show_matches);
  hideMatches = new MatchPatternSet(hide_matches);
}

::: browser/components/extensions/ext-pageAction.js:117
(Diff revision 4)
>          },
>        }));
> +
> +      // If the page action is only enabled in some URLs, do pattern matching in
> +      // the active tabs and update the button if necessary.
> +      if (show == null) {

Since the default is to hide, we only need to do this if there are showMatches.  So "if (showMatches)".

::: browser/components/extensions/ext-pageAction.js:195
(Diff revision 4)
> +  // Checks whether the tab action is shown when the specified tab becomes active.
> +  // Does pattern matching if necessary, and caches the result as a tab-specific value.
> +  isShown(tab) {
> +    let tabData = this.tabContext.get(tab);
> +    let {show} = tabData;
> +    if (show != null) {

!== undefined is fine here.  

However I think this function could look like:

if (tabData.show === undefined) {
  tabData.show = tabData.showMatches &&
    tabData.showMatches.matches(uri) &&
    !tabData.hideMatches.matches(uri);
}
return tabData.show

::: browser/components/extensions/test/browser/browser_ext_pageAction_show_matches.js:50
(Diff revision 4)
> +    {
> +      "name": "Test hide_matches all_urls overrides all_urls.",
> +      "page_action": {
> +        "show_matches": ["<all_urls>"],
> +        "hide_matches": ["<all_urls>"],
> +      },
> +      "shown": [false, false, false, false],
>      },

settings like this is nonsensical.  Given my other suggested changes, this test should have show_matches with all_urls same as the first test.  I don't think the extra test is necessary.

::: browser/components/extensions/test/browser/browser_ext_pageAction_show_matches.js:93
(Diff revision 4)
>          window.getComputedStyle(button).display == "none";
>        is(!hidden, test.shown[t], `test ${i} tab ${t}: page action is ${test.shown[t] ? "shown" : "hidden"} for ${tab.linkedBrowser.currentURI.spec}`);
> -    }
> +      await sendMessage(extension, "isShown", {result: test.shown[t]});
> +    };
>  
> +    info("Check show_matches and hide_matches are respected when installing the extension");

I think this test has become confusing, it's trying to do too much.  

It seems we have two things we want to verify.  The shown state of tabs on startup (active tab and  otherwise), and the state upon tab switching.

I'd split this in two test functions (perhaps just leave the original as-is but make the test data global).
Attachment #8949867 - Flags: review?(mixedpuppy) → review-
(In reply to Oriol Brufau [:Oriol] from comment #17)

> > >  
> > > -  onStateChange(browser, webProgress, request, stateFlags, statusCode) {

> So removing this code is good.

I understand why the str is broken and why the change fixes *that* str.  My point is that there is likely some other edge case that wasn't working until onStateChange was introduced.
(In reply to Shane Caraveo (:mixedpuppy) from comment #19)
> (In reply to Oriol Brufau [:Oriol] from comment #17)
> 
> > > >  
> > > > -  onStateChange(browser, webProgress, request, stateFlags, statusCode) {
> 
> > So removing this code is good.
> 
> I understand why the str is broken and why the change fixes *that* str.  My
> point is that there is likely some other edge case that wasn't working until
> onStateChange was introduced.

I am also open to the possibility there is not any reason for it, or that the reason no longer exists, but it needs a deeper look.
(In reply to Shane Caraveo (:mixedpuppy) from comment #18)
> Since hide_matches overrides, setting that to all_urls also creates (what
> should be) a no-op, but in a confusing manor.

Well, that's how this API was designed. I'm just fixing bugs. I'm surprised you don't like it since it seems it was you who designed it in bug 1419842 comment 18.

> The current situation is:
> 
> show_matches: ["http://example.com"],
> hide_matches: ["<all_urls>"]
> 
> will never result in the action being shown on example.com, but clearly the
> intention is that the action is shown on example.com.  Yes, the author
> should not have added hide_matches, but the point is that we should always
> honor show_matches in this case.
> IMO Just remove all_urls from hide_matches (logging a warning is fine).

I don't think we should assume webext authors are stupid and they meant different things than what they wrote.
If an author does not know how hide_matches works, then it's better that we don't try to fix things so that they will notice the problem earlier.
So I'm against ignoring <all_urls>, even with a warning. Blacklisting it so that the webext does not load if used in hide_matches would be more acceptable, but I don't see the need to do that.
If you think some authors would prefer show_matches to have more precedence than hide_matches in some cases, then I think you should propose some kind of manifest flag to do that.

That is, since

  show_matches: ["http://example.com/"],
  hide_matches: ["*://example.com/*"]

is no-op, then I think

  show_matches: ["http://example.com/"],
  hide_matches: ["<all_urls>"]

this should also be no-op instead of magically showing the page action in http://example.com/
Otherwise it's not consistent.

> As well, if there are no show_matches, then hide_matches shouldn't have any
> effect.

Yes, this is what happens already.

> ::: browser/components/extensions/ext-pageAction.js:117
> (Diff revision 4)
> >          },
> >        }));
> > +
> > +      // If the page action is only enabled in some URLs, do pattern matching in
> > +      // the active tabs and update the button if necessary.
> > +      if (show == null) {
> 
> Since the default is to hide, we only need to do this if there are
> showMatches.  So "if (showMatches)".

Uhm, in fact showMatches is thuthy if, and only if, the default show is null.
So it doesn't really matter, but not sure if I follow your reasoning.
For consistency I would prefer show===null since this is what must be checked in tab-specific show.

> ::: browser/components/extensions/ext-pageAction.js:195
> (Diff revision 4)
> > +  // Checks whether the tab action is shown when the specified tab becomes active.
> > +  // Does pattern matching if necessary, and caches the result as a tab-specific value.
> > +  isShown(tab) {
> > +    let tabData = this.tabContext.get(tab);
> > +    let {show} = tabData;
> > +    if (show != null) {
> 
> !== undefined is fine here.  

In the last update I switched to null. Do you want !==null or should I revert to undefined and use !==undefined?

> However I think this function could look like:
> 
> if (tabData.show === undefined) {
>   tabData.show = tabData.showMatches &&
>     tabData.showMatches.matches(uri) &&
>     !tabData.hideMatches.matches(uri);
> }
> return tabData.show

If tabData.show is not a boolean then tabData.showMatches must be truthy, no need to check that.
I was trying to avoid reading tabData.show multiple times, since it can be defined up in the prototype chain, but I guess it doesn't really matter.

> :::
> browser/components/extensions/test/browser/
> browser_ext_pageAction_show_matches.js:50
> (Diff revision 4)
> > +    {
> > +      "name": "Test hide_matches all_urls overrides all_urls.",
> > +      "page_action": {
> > +        "show_matches": ["<all_urls>"],
> > +        "hide_matches": ["<all_urls>"],
> > +      },
> > +      "shown": [false, false, false, false],
> >      },
> 
> settings like this is nonsensical.  Given my other suggested changes, this
> test should have show_matches with all_urls same as the first test.  I don't
> think the extra test is necessary.

On the contrary, I think it's really necessary. Especially if you want <all_urls> in hide_matches to be handled in some special way.
(In reply to Oriol Brufau [:Oriol] from comment #21)
> (In reply to Shane Caraveo (:mixedpuppy) from comment #18)
> > Since hide_matches overrides, setting that to all_urls also creates (what
> > should be) a no-op, but in a confusing manor.
> 
> Well, that's how this API was designed. I'm just fixing bugs. I'm surprised
> you don't like it since it seems it was you who designed it in bug 1419842
> comment 18.

As the implementer I'm trying to explain the intention behind this so we can address it in a way that removes the potential confusion.  I'd like to see all_urls removed from hide_matches one way or another.  The schema can be modified to disallow all_urls which is probably the better way to go.  Here's the change to make that work.  Not fond of "MatchPatternRestricted" but not sure what a better name is right now.

diff --git a/browser/components/extensions/schemas/page_action.json b/browser/components/extensions/schemas/page_action.json
--- a/browser/components/extensions/schemas/page_action.json
+++ b/browser/components/extensions/schemas/page_action.json
@@ -37,17 +37,17 @@
                 "optional": true,
                 "minItems": 1,
                 "items": { "$ref": "MatchPattern" }
               },
               "hide_matches": {
                 "type": "array",
                 "optional": true,
                 "minItems": 1,
-                "items": { "$ref": "MatchPattern" }
+                "items": { "$ref": "MatchPatternRestricted" }
               }
             },
             "optional": true
           }
         }
       }
     ]
   },
diff --git a/toolkit/components/extensions/schemas/manifest.json b/toolkit/components/extensions/schemas/manifest.json
--- a/toolkit/components/extensions/schemas/manifest.json
+++ b/toolkit/components/extensions/schemas/manifest.json
@@ -431,16 +431,29 @@
           },
           {
             "type": "string",
             "pattern": "^file:///.*$"
           }
         ]
       },
       {
+        "id": "MatchPatternRestricted",
+        "description": "Same as MatchPattern above, but excludes <all_urls>",
+        "choices": [
+          {
+            "type": "string",
+            "pattern": "^(https?|wss?|file|ftp|\\*)://(\\*|\\*\\.[^*/]+|[^*/]+)/.*$"
+          },
+          {
+            "type": "string",
+            "pattern": "^file:///.*$"
+          }
+        ]
+      },
+      {
         "id": "MatchPatternInternal",
         "description": "Same as MatchPattern above, but includes moz-extension protocol",
         "choices": [
           {
             "type": "string",
             "enum": ["<all_urls>"]
           },
           {
I refactored the test, used strict equalities and changed the isShown function.

Now I will do what you say in comment 22.
I thought reusing MatchPatternRestricted in MatchPattern was better, but I will do your proposal if you prefer it
Comment on attachment 8949867 [details]
Bug 1437178 - Fix various pageAction visibiltiy issues when using show_matches and hide_matches

https://reviewboard.mozilla.org/r/219186/#review225752

r- only due to the onStateChange comment below.  Other stuff is just a couple comment cleanups.

::: browser/components/extensions/ext-browser.js
(Diff revision 6)
>        this.emit("tab-select", nativeTab);
>        this.emit("location-change", nativeTab);
>      }
>    }
>  
> -  onStateChange(browser, webProgress, request, stateFlags, statusCode) {

Ok, so this was added in bug 1287649.  Based on the patch in that bug, I don't see an obvious reason this would cause a problem with the show/hide patch here, but the particular reason for the "lastLocation" stuff is important, we don't want to regress that.

::: browser/components/extensions/ext-pageAction.js:50
(Diff revision 6)
> -    let showMatches = new MatchPatternSet(options.show_matches || []);
> -    let hideMatches = new MatchPatternSet(options.hide_matches || []);
> +    //   It's set as default if show_matches is empty. Can also be set in a tab via
> +    //   `pageAction.hide(tabId)`, e.g. in order to override show_matches.
> +    // - `true`. This means the page action is shown.
> +    //   It's never set as default because <all_urls> doesn't really match all URLs
> +    //   (e.g. "about:" URLs). But can be set in a tab via `pageAction.show(tabId)`.
> +    // - `null`. This means the visivility of the page action may depends on the tab URL.

First sentence is unecessary, the following two sentences are quite specific and enough.

::: browser/components/extensions/ext-pageAction.js:190
(Diff revision 6)
>      }
>      this.browserPageAction.setIconURL(iconURL, window);
>    }
>  
> +  // Checks whether the tab action is shown when the specified tab becomes active.
> +  // Does pattern matching if necessary, and caches the result as a tab-specific value.

Just add a comment that show can only be null if matches were configured.
Attachment #8949867 - Flags: review?(mixedpuppy) → review-
(In reply to Shane Caraveo (:mixedpuppy) from comment #27)
> Ok, so this was added in bug 1287649.  Based on the patch in that bug, I
> don't see an obvious reason this would cause a problem with the show/hide
> patch here, but the particular reason for the "lastLocation" stuff is
> important, we don't want to regress that.

OK, I will look into that. Or maybe this should be moved into another bug, but without removing that code my test failed.
I'm not sure if the test from bug 1287649 works well. My patch didn't fail everywhere in treeherder, and where it failed it was marked as intermittent.
> ::: browser/components/extensions/ext-pageAction.js:50
> (Diff revision 6)
> > -    let showMatches = new MatchPatternSet(options.show_matches || []);
> > -    let hideMatches = new MatchPatternSet(options.hide_matches || []);
> > +    //   It's set as default if show_matches is empty. Can also be set in a tab via
> > +    //   `pageAction.hide(tabId)`, e.g. in order to override show_matches.
> > +    // - `true`. This means the page action is shown.
> > +    //   It's never set as default because <all_urls> doesn't really match all URLs
> > +    //   (e.g. "about:" URLs). But can be set in a tab via `pageAction.show(tabId)`.
> > +    // - `null`. This means the visivility of the page action may depends on the tab URL.
> 
> First sentence is unecessary, the following two sentences are quite specific
> and enough.

Looks like some context is missing in my comment above.  This is about the comments for "null".  There are two more sentences after the last line rb sent.
(In reply to Shane Caraveo (:mixedpuppy) from comment #29)
> Looks like some context is missing in my comment above.  This is about the
> comments for "null".  There are two more sentences after the last line rb
> sent.

So you want me to remove "This means the visivility of the page action may depends on the tab URL.", right? It has a typo anyways
(In reply to Oriol Brufau [:Oriol] from comment #30)
> (In reply to Shane Caraveo (:mixedpuppy) from comment #29)
> > Looks like some context is missing in my comment above.  This is about the
> > comments for "null".  There are two more sentences after the last line rb
> > sent.
> 
> So you want me to remove "This means the visivility of the page action may
> depends on the tab URL.", right? It has a typo anyways

yes
(In reply to Oriol Brufau [:Oriol] from comment #28)
> (In reply to Shane Caraveo (:mixedpuppy) from comment #27)
> > Ok, so this was added in bug 1287649.  Based on the patch in that bug, I
> > don't see an obvious reason this would cause a problem with the show/hide
> > patch here, but the particular reason for the "lastLocation" stuff is
> > important, we don't want to regress that.
> 
> OK, I will look into that. Or maybe this should be moved into another bug,
> but without removing that code my test failed.
> I'm not sure if the test from bug 1287649 works well. My patch didn't fail
> everywhere in treeherder, and where it failed it was marked as intermittent.

No, we don't remove necessary code to land something.  It needs to be validated as unnecessary (with tests) or fixed (can be fixed in a separate bug, but as a blocker to landing this patch).  I also did not like the looks of the test in that bug, but didn't dig further.
(In reply to Shane Caraveo (:mixedpuppy) from comment #32)
> No, we don't remove necessary code to land something.  It needs to be
> validated as unnecessary (with tests) or fixed (can be fixed in a separate
> bug, but as a blocker to landing this patch).  I also did not like the looks
> of the test in that bug, but didn't dig further.

Yes, I meant this bug could land the other fixes and then in another bug fix that the pageAction and browserAction are not always updated when they should. Or in reversed order.
(In reply to Oriol Brufau [:Oriol] from comment #33)
> (In reply to Shane Caraveo (:mixedpuppy) from comment #32)
> > No, we don't remove necessary code to land something.  It needs to be
> > validated as unnecessary (with tests) or fixed (can be fixed in a separate
> > bug, but as a blocker to landing this patch).  I also did not like the looks
> > of the test in that bug, but didn't dig further.
> 
> Yes, I meant this bug could land the other fixes and then in another bug fix
> that the pageAction and browserAction are not always updated when they
> should. Or in reversed order.

Ah, ok.  Problem is, if your tests don't pass (due to the other bug) it will end up getting backed out.  So it's best to address the other issue and land it first.
Depends on: 1438274
Priority: -- → P2
Comment on attachment 8949867 [details]
Bug 1437178 - Fix various pageAction visibiltiy issues when using show_matches and hide_matches

https://reviewboard.mozilla.org/r/219186/#review226638

Given that bug 1438274 clears up the onStateChange issue, I think this is good after resolving any last nits.
Attachment #8949867 - Flags: review- → review+
(In reply to Shane Caraveo (:mixedpuppy) from comment #35)
> Given that bug 1438274 clears up the onStateChange issue, I think this is
> good after resolving any last nits.

In fact it needs some further changes, because since bug 1438274 it's possible for the location to change without clearing the tab data. This means the result of pattern matching can't be stored as "show", because we need to distinguish the result of a previous pattern matching that might be now obsolete from the value store by show() or hide().
So now "show" contains the value set by show(), hide() or set by default because of an empty show_matches. And "patternMatching" caches the result of pattern matching, which has less precedence.

Choosing between them was easier if using a destructuring assignment with a default, so the not-set value is now undefined instead of null. But I still used strict equalities.

I added a test about history.pushState
Comment on attachment 8949867 [details]
Bug 1437178 - Fix various pageAction visibiltiy issues when using show_matches and hide_matches

Try seems green
Attachment #8949867 - Flags: review+ → review?(mixedpuppy)
Comment on attachment 8949867 [details]
Bug 1437178 - Fix various pageAction visibiltiy issues when using show_matches and hide_matches

https://reviewboard.mozilla.org/r/219186/#review226914

LGTM! Thanks!

::: browser/components/extensions/ext-pageAction.js:177
(Diff revision 7)
>      this.browserPageAction.setTitle(title, window);
>      this.browserPageAction.setTooltip(title, window);
> -    this.browserPageAction.setDisabled(!tabData.show, window);
>  
> +    // At least one of "show" or "patternMatching" must be defined here.
> +    let {show = tabData.patternMatching} = tabData;

This does require that one of those has been set prior to updateButton, otherwise it can result in show === undefined.  However that results in the button being disabled, so I'm not really concerned.
Attachment #8949867 - Flags: review?(mixedpuppy) → review+
(In reply to Shane Caraveo (:mixedpuppy) from comment #40)
> This does require that one of those has been set prior to updateButton,
> otherwise it can result in show === undefined.  However that results in the
> button being disabled, so I'm not really concerned.

Yes, calling isShown ensures one will be defined. The only case where updateButton is called without checking isShown is in setProperty, but this only happens if the tab is active. And I covered both cases of becoming active and being already active when the extension is installed. But yes, if I missed some edge case, the worst that will happen is that the button will be hidden.

Bug 1438274 landed, let's land this now.
Keywords: checkin-needed
Pushed by archaeopteryx@coole-files.de:
https://hg.mozilla.org/integration/autoland/rev/a46fa982caed
Fix various pageAction visibiltiy issues when using show_matches and hide_matches r=mixedpuppy
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/a46fa982caed
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Should this (and bug 1438274) be uplifted to fix the initial implementation of show_matches and hide_matches, or do you prefer to ride the trains?
Flags: needinfo?(mixedpuppy)
Request uplifts on both.
Flags: needinfo?(mixedpuppy)
Attached file test3.xpi
Attachment #8949863 - Attachment is obsolete: true
Attachment #8950324 - Attachment is obsolete: true
Attached file test4.xpi
Steps to reproduce:

1. Disable xpinstall.signatures.required in about:config
2. Load https://bugzilla.mozilla.org in a tab
3. In that tab, go to https://bugzilla.mozilla.org/attachment.cgi?id=8952160
4. Install the extension

Expected: You should see the page action just after install (the location matches show_matches)

5. Open a new tab
6. Load https://bugzilla.mozilla.org

Expected: You should see the page action (the location matches show_matches)

7. Remove the extension in about:addons and restore xpinstall.signatures.required
8. In about:debugging, load the attached test4.xpi

Expected: You should not see the page action in about:debugging (the location does not match show_matches)

9. Open a new tab
10. Load http://example.com/

Expected: You should not see the page action in about:debugging (the location matches hide_matches)
Comment on attachment 8949867 [details]
Bug 1437178 - Fix various pageAction visibiltiy issues when using show_matches and hide_matches

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1419842
[User impact if declined]: show_matches and hide_matches won't work properly in some cases. They were introduces in Firefox 59, so it would be nice to fix the initial implementation.
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: I have manually verifies in nightly
[Needs manual test from QE? If yes, steps to reproduce]: See comment 47
[List of other uplifts needed for the feature/fix]: bug 1438274 should be uplifted first.
[Is the change risky?]: Not much risky
[Why is the change risky/not risky?]: The patch properly handles show_matches and hide_matches in various common situations. Even if I missed some rare edge case, it won't be worse than now.
[String changes made/needed]: None
Attachment #8949867 - Flags: approval-mozilla-beta?
Summary: pageAction visibiltiy issues when using show_matches and hide_matches → pageAction visibility issues when using show_matches and hide_matches
Comment on attachment 8949867 [details]
Bug 1437178 - Fix various pageAction visibiltiy issues when using show_matches and hide_matches

Fix for extension page actions, let's uplift for beta 12.
Attachment #8949867 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
I was able to reproduce this issue on Nightly 60.0a1 (2018-02-09) under Windows 10 x64 by following the steps from comment 47. 
I retested everything on Nightly 60.0a1 (2018-02-25) and Beta 59.0b12 (20180222170353) under Windows 10 x64, macOS 10.13 and Ubuntu 16.04 x32 and I can confirm this issue is fixed.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Depends on: 1447723
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.