1493470 - pageAction icon goes hidden respond to a cross-site domain's movie inside iframe

Status: VERIFIED FIXED

(WebExtensions :: General, defect)

Description

[prsprsbk]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:63.0) Gecko/20100101 Firefox/63.0
Build ID: 20180920135444

Steps to reproduce:

I use this addon, on Firefox Developer Edition.
https://addons.mozilla.org/en-US/firefox/addon/prsprstwitter/

I visited twitter. (https://twitter.com)
Respond to url, pageAction was shown (not by pageAction.openPopup() API) on location bar at first.


Actual results:

Click and play YouTobe Movie inside a tweet.
pageAction icon became hidden, and it could not recover by operation on location bar.
When webpage was reloaded by F5 Key, pageAction icon appeared again.
And in the case that a movie's source was on twitter domain, pageAction icon had been kept shown.

About content script, it was still running (that addon automatically click 'show new tweet', so I could discriminate).


Expected results:

pageAction should not be hidden, because Page actions are for paticular page as descripted in following MDN document, and not for contents inside iframes.
https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/pageAction

Or if contents inside iframes are also target and there exists some rules as to domain, it is necessary for background script to detect such event.
Currently browser.tabs.onUpdate() does not detect event, and maybe not appropriate for it.

Comment 1

[Damira Akh]

Build ID 	20180926220146
User Agent 	Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:64.0) Gecko/20100101 Firefox/64.0

I was able to reproduce this issue.
'
Placing it in WebExtensions: General if it's not correct component please feel free to change.
Thank you

Comment 2

[David Durst [:ddurst]]

PageAction is reset on history changes.

Comment 3

[prsprsbk]

(In reply to David Durst [:ddurst] (Regression Engineering Owner for 63) from comment #2)
> PageAction is reset on history changes.

If 'history' means the one managing history and bookmarks in the menu, no change is added.
And I tried searching by Ctrl-F with 'youtube' over that history, there was no match.
Where should I go to know history changes?
Thanks in advance.

Comment 4

[prsprsbk]

(In reply to David Durst [:ddurst] (Regression Engineering Owner for 63) from comment #2)
> PageAction is reset on history changes.

Sorry let me drop my question, because history change is invalid as a reason and this need to re-open.
If you had began some work, excuse me.

I thought GUI was hiding browsing history, but places.sqlite showed that no history change had happened.
I deleted my browsing history at all, and just had issue reproduced.

--------------------
pwsh:$ sqlite3.exe D:\places.sqlite
SQLite version 3.25.1 2018-09-18 20:20:44
Enter ".help" for usage hints.
sqlite> SELECT COUNT(*) FROM moz_historyvisits;
2
sqlite> SELECT * FROM moz_historyvisits;
1|0|455|1538523754284000|3|0
2|1|237348|1538523768668000|1|0

sqlite> SELECT id,url,visit_count FROM moz_places WHERE id = 455 OR id = 237348;
455|https://twitter.com/|1
237348|https://twitter.com/search?q=youtube%20from%3APrsPrsBK&src=typd|1

sqlite> select id,url,visit_count from moz_places where url like '%youtube%' order by id desc limit 10;
237348|https://twitter.com/search?q=youtube%20from%3APrsPrsBK&src=typd|1
sqlite>
--------------------

So proposed reason is invalid for closing this issue, and I later re-open this, with reshaping as Page Action reset issue (this expression is right and thanks), not as hidden icon.

Comment 5

[Luca Greco [:rpl] [:luca] [:lgreco]]

(In reply to prsprsbk from comment #3)
> (In reply to David Durst [:ddurst] (Regression Engineering Owner for 63)
> from comment #2)
> > PageAction is reset on history changes.
> 
> If 'history' means the one managing history and bookmarks in the menu, no
> change is added.
> And I tried searching by Ctrl-F with 'youtube' over that history, there was
> no match.
> Where should I go to know history changes?
> Thanks in advance.

The pageAction used to be cleared when a website was adding history entries using history.pushState, 
but I checked how we are handling this scenario right now and it looks that we are now avoiding that
explicitly:

- https://searchfox.org/mozilla-central/rev/a11c128b90ea85d7bb68f00c5de52c0794e0b215/browser/components/extensions/parent/ext-browser.js#194-195
- https://searchfox.org/mozilla-central/rev/a11c128b90ea85d7bb68f00c5de52c0794e0b215/browser/components/extensions/parent/ext-pageAction.js#317-320

I investigated this issue locally, and it looks like there is a bug to fix:

TabContext.onLocationChange is not currently checking the webProgress.isTopLevel property
to be sure that the navigation  is related to a top level frame navigation (and not one of its sub frames), 
and that explains why by playing a youtube video embedded into a twitter page the pageAction is being cleared:

- https://searchfox.org/mozilla-central/rev/a11c128b90ea85d7bb68f00c5de52c0794e0b215/browser/components/extensions/parent/ext-browser.js#191-197

As an additional confirmation, we do check the webProgress.isTopLevel in the internal waitForTabLoaded helper: https://searchfox.org/mozilla-central/rev/a11c128b90ea85d7bb68f00c5de52c0794e0b215/browser/components/extensions/parent/ext-browser.js#112 and internally the tabbrowser (which is the component from which we are being notified for the location changes) also check it here https://searchfox.org/mozilla-central/rev/a11c128b90ea85d7bb68f00c5de52c0794e0b215/browser/base/content/tabbrowser.js#4951-4953, but it also calls the onLocationChange progress listeners when the location change is not a top level one (and so it leaves to the tab progress listener the choice if it should handle a subframe location change or not).

I'm re-opening this issue and I'm going to attach a patch to it asap.

Comment 6

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: Bug 1493470 - pageAction icon should not be reset on subframe navigations. r?mixedpuppy

Comment 7

[Luca Greco [:rpl] [:luca] [:lgreco]]

The patch attached in comment 6, besides including a proposed fix, it also includes a new test case to trigger
the issue and check the behaviors before and after the applied changes.

The following try push is related to a build that includes the new test case without the proposed fix:
- https://treeherder.mozilla.org/#/jobs?repo=try&revision=911edbebda6783807cb91cf711eeb61cf0363f5b

Whereas the following one is related to a build which includes both the new test case and the proposed fix:
- https://treeherder.mozilla.org/#/jobs?repo=try&revision=657e971aee9835ccf71525eaccfd907f93594ca5

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/22788736d036
pageAction icon should not be reset on subframe navigations. r=mixedpuppy

Comment 9

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/22788736d036

Comment 10

[CosminB]

Attachment: Bug1493470.gif

I was able to reproduce this issue on Firefox 62.0.3(20181001155545) and Firefox 63.0b12(20181004174654) under Win 7 64-bit and  Mac OS X 10.13.3.

This issue is verified as fixed on Firefox 64.0a1(20181004224156) under Win 7 64-bit and Mac OS X 10.13.3.

Please see the attached video.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Is this something we should consider for Beta uplift or can it ride the trains?

Comment 12

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 9014013 [details]
Bug 1493470 - pageAction icon should not be reset on subframe navigations. r?mixedpuppy

I've been (with some help from QA) double-checking if this was a regression or if the issue was there from the start.

Personally I quickly tested it on Firefox 60 ESR and the issue was already there, then QA tried to bisect it using mozregression and it seems that the issue was already there at least starting from Firefox 56 (and I'm not sure that it is really worth to check the range between 45 and 56 at this point).

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: None

User impact if declined: The pageAction and browser action context for a tab (e.g. the icon set using setIcon) will be cleared when a subframe navigates.

(there isn't a bug id that caused the regression, as it seems that the pageAction.setIcon has been working that way for a while, at least from Firefox 56).

Is this code covered by automated tests?: Yes

Has the fix been verified in Nightly?: Yes

Needs manual test from QE?: Yes

If yes, steps to reproduce: It can be verified using the STR from comment 0 (which is same one used to verify it on nightly)

List of other uplifts needed: None

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): The actual change is pretty small, it is applied on a part that is only used by pageAction and browserAction to reset the tab context on navigation (and so it should not affect any other part of the webextensions internals), and it is tested in automation.

String changes made/needed: None

Comment 13

[Pascal Chevrel:pascalc (PTO until April 26)]

Comment on attachment 9014013 [details]
Bug 1493470 - pageAction icon should not be reset on subframe navigations. r?mixedpuppy

Although the patch is minimal and has tests, our last beta before RC week is tomorrow and since you mention in your uplift request that we have this bug since at least Firefox 56, I think that it should just ride the trains to 64. Thank you!

Comment 14

[Brindusa Tot, Desktop QA]

Based on previous comments, I'm removing the qe-verify+ flag.

Comment 15

[prsprsbk]

Thanks for all work very much.
Let me add little and limited workaround.
show_matches property (since Firefox 59) of page_action key in manifest.json, it prevents icon from being hidden (still effects by background script is reset at Firefox 63).
I noticed it recently (so I could not mention it), and I updated my addon to use it. thanks.