Closed Bug 1438274 Opened 3 years ago Closed 3 years ago

browser and page actions not cleared properly when navigating

Categories

(WebExtensions :: General, defect, P2)

50 Branch
defect

Tracking

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

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

People

(Reporter: Oriol, Assigned: Oriol)

References

Details

(Keywords: regression)

Attachments

(2 files)

Attached file test.xpi
1. Load http://example.com in a tab
2. Go to about:debugging in another tab, and load the attached extension
3. Go back to the http://example.com tab
4. Click the browser action, this shows a badge
5. Add a hash so that the url becomes http://example.com#hash

Expected: The badge is not cleared because no navigation occurred
Result: The badge is cleared because lastLocation from bug 1287649 doesn't have data about the tab (because the extension was just installed), so it thinks it's a new location

6. Open a new tab
7. Click the browser action, this shows a badge
8. Navigate to http://example.com

Expected: The badge is cleared because navigation occurred
Result: The badge is not cleared because of the onStateChange hack from bug 1287649

This is important for page actions, the visibility can change depending on the URL so a location-change event should be sent. Bug 1437178 needs this to work correctly.

9. Open a new tab
10. Load http://example.com/#foo
11. Click the browser action, this shows a badge
12. Remove the hash so that the url becomes http://example.com

Expected: The badge is cleared because navigation occurred
Result: The badge is not cleared because equalsExceptRef from bug 1287649 returns true

Chrome does them all correctly.
Assignee: nobody → oriol-bugzilla
Status: NEW → ASSIGNED
More steps:

13. Open a new tab with http://example.com/
14. Click the browser action, this shows a badge
15. Open the console and run history.pushState(null, null, "http://example.com/path")

Expected: The badge is not cleared because no navigation occurred
Result: The badge is cleared

This was intentional in bug 1287649 comment 4. But I object, tab-specific data should only be cleared if there was navigation. As expected, Chrome does not clear it.

So my patch only sends a location-change event with fromBrowse=true if the LOCATION_CHANGE_SAME_DOCUMENT flag is not present.
However, if the URL changes, page actions must still be notified because the visibility can depend on the URL via show_matches and hide_matches. So in this case I send a location-change event with fromBrowse=false (previously this only happened when switching tabs).
Comment on attachment 8951286 [details]
Bug 1438274 - Fix browser and page actions clearance when navigating

https://reviewboard.mozilla.org/r/220534/#review226506

This looks good, just a couple small changes I noted below.

::: browser/components/extensions/test/browser/browser_ext_pageAction_context.js
(Diff revision 1)
>            browser.pageAction.setPopup({tabId, popup: "2.html"});
>            browser.pageAction.setTitle({tabId, title: "Title 2"});
>  
>            expect(details[2]);
>          },
> -        async expect => {

If this test were left in, would it pass?  If so, lets leave it.  If not, why not?

::: browser/components/extensions/test/browser/browser_ext_pageAction_context.js:250
(Diff revision 1)
> +  await extension.startup();
> +
> +  info("Set tab-specific data to the existing tab.");
> +  await setTabSpecificData();
> +
> +  info("Add a hash. Does not cause navigation, tab-specific data is not cleared.");

For the expect tests, pass these strings through so they are the strings used in assertEq.
Attachment #8951286 - Flags: review?(mixedpuppy) → review+
Comment on attachment 8951286 [details]
Bug 1438274 - Fix browser and page actions clearance when navigating

(In reply to Shane Caraveo (:mixedpuppy) from comment #3)
> If this test were left in, would it pass?  If so, lets leave it.  If not,
> why not?

It fails, not sure why. When I test manually tab data is not cleared.
The current test fails intermittently lots of times (bug 1405453), so I think it's better to just remove it.
Attachment #8951286 - Flags: review+ → review?(mixedpuppy)
Comment on attachment 8951286 [details]
Bug 1438274 - Fix browser and page actions clearance when navigating

For some reason browser_ext_pageAction_title.js is almost a copy of browser_ext_pageAction_context.js
Attachment #8951286 - Flags: review?(mixedpuppy)
(In reply to Oriol Brufau [:Oriol] from comment #5)
> Comment on attachment 8951286 [details]
> Bug 1438274 - Fix browser and page actions clearance when navigating
> 
> (In reply to Shane Caraveo (:mixedpuppy) from comment #3)
> > If this test were left in, would it pass?  If so, lets leave it.  If not,
> > why not?
> 
> It fails, not sure why. When I test manually tab data is not cleared.
> The current test fails intermittently lots of times (bug 1405453), so I
> think it's better to just remove it.

Either it fails because how or what it is testing is wrong, or its testing something that should work but isn't.  But we don't land the fix removing the test without answering that first.

The intermittents are low so it's probably some timing issue not accounted for, I'm not really concerned on that right now.
OK, I think I get it. The problem is that even if you await browser.tabs.create, the url is already not loaded. The tab is still in about:blank. So when the tab navigates from about:blank to the specified url (about:blank?0 in that test), the data is cleared.
I think browser.tabs.create shouldn't resolve until the url has changed. Otherwise it's a bit strange:

    let tab = await browser.tabs.create({active: true, url: "http://example.com"});
    console.log(tab.url); // "about:blank"
Attachment #8951286 - Flags: review?(mixedpuppy)
Comment on attachment 8951286 [details]
Bug 1438274 - Fix browser and page actions clearance when navigating

https://reviewboard.mozilla.org/r/220534/#review226636

Good catch on those tests.
Attachment #8951286 - Flags: review?(mixedpuppy) → review+
(In reply to Oriol Brufau [:Oriol] from comment #9)
> OK, I think I get it. The problem is that even if you await
> browser.tabs.create, the url is already not loaded. The tab is still in
> about:blank. So when the tab navigates from about:blank to the specified url
> (about:blank?0 in that test), the data is cleared.

It's expected that the data is cleared because it's a navigation.
But various other tests (and probably extension authors) don't expect the data to be cleared if they set it just after browser.tabs.create
So I guess this should be fixed in another bug, blocking this one.
Depends on: 1438672
(In reply to Oriol Brufau [:Oriol] from comment #12)

> It's expected that the data is cleared because it's a navigation.
> But various other tests (and probably extension authors) don't expect the
> data to be cleared if they set it just after browser.tabs.create
> So I guess this should be fixed in another bug, blocking this one.

As you probably saw in the bug you made, it's expected behavior.
No longer depends on: 1438672
OK, I thought fixing this without fixing bug 1438672 would make Firefox incompatible with Chrome, but it's the opposite. Chrome also clears tab-specific data after browser.tabs.create(). So I guess I will only need to fix all the tests that failed in treeherder.
> Create a new tab.
> Await tab load. No icon visible.
> Buffered messages finished
> TEST-UNEXPECTED-FAIL | browser/components/extensions/test/browser/browser_ext_pageAction_title.js | Test timed out - 

Oh. So sometimes the the tab is already loaded just after browser.tabs.create()
So unreliable. Fixing the tests won't be as easy as I thought.
Try seems green now
Keywords: checkin-needed
Oriol, the patch here still has the changes for onstatechange.  This patch should be rebased on top of bug 1438672
argh, so sorry, got my bugs crossed.
Keywords: checkin-needed
Looks like this will land, so assigning it an appropriate priority.
Priority: -- → P2
Pushed by csabou@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/81617ea852ba
Fix browser and page actions clearance when navigating r=mixedpuppy
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/81617ea852ba
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Comment on attachment 8951286 [details]
Bug 1438274 - Fix browser and page actions clearance when navigating

Approval Request Comment
[Feature/Bug causing the regression]: bug 1287649
[User impact if declined]: webextension browser and page actions may be cleared when they shouldn't, or not cleared when they should. And bug 1437178 can't be uplifted without uplifting this first.
[Is this code covered by automated tests?]: Yes.
[Has the fix been verified in Nightly?]: I have manually verified in Nightly.
[Needs manual test from QE? If yes, steps to reproduce]: See comment 0 and comment 1
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: not much risky
[Why is the change risky/not risky?]: Detects navigation using reliable Ci.nsIWebProgressListener.LOCATION_CHANGE_SAME_DOCUMENT flag instead of WeakMap workarounds. The other changes are test fixes and new tests.
[String changes made/needed]: none
Attachment #8951286 - Flags: approval-mozilla-beta?
Too late for 58 and not the kind of security/stability fix we would uplift to esr52 at this point. But we can take it for 59 beta.
Comment on attachment 8951286 [details]
Bug 1438274 - Fix browser and page actions clearance when navigating

Let's uplift for beta 12. Thanks for the new tests.
Attachment #8951286 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
I have reproduced this issue using Firefox 60.0a1 (2018.02.14) on Win 8.1 x64 based on comment 0, steps 9-12.
I can confirm this issue is fixed, I verified using Firefox 59.0b12 and 60.0a1 (2018.02.25) on Win 8.1 x64, Mac OS X 10.13.4 and Ubuntu 14.04 x64.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.