1500479 - Enhance browser.tabs API to support assigning tab successors

Status: RESOLVED FIXED

(WebExtensions :: Frontend, enhancement, P3)

Description

[Ryan Hendrickson [rhendric on GitLab, GitHub]]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Firefox/60.0

Steps to reproduce:

See bug 1419947 for more discussion. I introduced the concept of tab successors into tabBrowser in a patch attached to that bug; this bug is for tracking the WebExtensions API changes needed to enable the use cases described in that bug.

Comment 1

[Ryan Hendrickson [rhendric on GitLab, GitHub]]

Attachment: Bug 1500479 - Part 1: browser.tabs.onActivated; r?mixedpuppy,rpl,JanH

Add an optional previousTabId property to the onActivated event,
which is present if the previously activated tab is still open.

Comment 2

[Ryan Hendrickson [rhendric on GitLab, GitHub]]

Attachment: Bug 1500479 - Part 2: expose tab successors in browser.tabs; r?mixedpuppy,rpl

1. Add successorId to the Tab type, so that it will be returned in, e.g.,
   browser.tabs.get calls

2. Extend or create the following methods on the browser.tabs API:
  - update: add successorTabId as an optional property on the provided
    updateProperties object
  - moveInSuccession: new method that manipulates tab successors in bulk
  - moveInSuccessionAfter: ditto

Depends on D9271

Comment 3

[Ryan Hendrickson [rhendric on GitLab, GitHub]]

Please note that these two patches don't have any tests yet; I'm looking for some early feedback on the design here while I figure out how to test them.

Comment 4

[Ryan Hendrickson [rhendric on GitLab, GitHub]]

Both patches now have tests and are ready to be fully reviewed.

Comment 5

[Ryan Hendrickson [rhendric on GitLab, GitHub]]

Hey, I think this is more or less ready to be landed—I addressed all outstanding feedback on the patches and the reviewers gave provisional approval on a previous version. Can someone do a final sanity check and nudge this forward?

Comment 6

[Shane Caraveo (:mixedpuppy)]

Oh, that's been sitting a while.  Are you able to run a try before landing?

Make use of needinfo, it would have got on someones radar faster.

Comment 7

[Dão Gottwald [:dao]]

I tried to land this earlier today but the patch failed to apply: https://lando.services.mozilla.com/D9271/

Comment 8

[Ryan Hendrickson [rhendric on GitLab, GitHub]]

Both patches have been rebased; try again please?

(I don't think I have access to run tries?)

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/02ea631f55c3
Part 1: browser.tabs.onActivated; r=mixedpuppy,rpl,JanH

Comment 10

[Dão Gottwald [:dao]]

*Sigh* Apparently only part 1 got landed. Is this self-contained or is it going to fail on automation without part 2?

Comment 11

[Ryan Hendrickson [rhendric on GitLab, GitHub]]

Part 1 should pass on its own (although I haven't actually tested it on its own since my first version of that patch).

Comment 12

[Dão Gottwald [:dao]]

Okay, I'll try to land part 2 separately now.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6f209bee42c5
Part 2: expose tab successors in browser.tabs; r=mixedpuppy,rpl

Comment 14

[Andreea Pavel [:apavel]]

backed out for failing bc at browser/components/extensions/test/browser/browser_ext_tabs_events.js and mochitest at browser/components/extensions/test/mochitest/test_ext_all_apis.html

Pushes that started the failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&&revision=02ea631f55c3ad689fff3e24df75e810cbe042ad and https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=6f209bee42c5a07163c89d21beb7e7062e2c75b1

Failure logs: bc: https://treeherder.mozilla.org/logviewer.html#?job_id=211733807&repo=autoland&lineNumber=2500

mochitest: https://treeherder.mozilla.org/logviewer.html#?job_id=211737870&repo=autoland&lineNumber=1640

backout: https://hg.mozilla.org/integration/autoland/rev/438b2437a3a7dbc542811c79f11144e8b6b62fbf

Comment 15

[Ryan Hendrickson [rhendric on GitLab, GitHub]]

I fixed the problems flagged in those logs; once more?

Comment 16

[Shane Caraveo (:mixedpuppy)]

(In reply to ryan.hendrickson from comment #15)
> I fixed the problems flagged in those logs; once more?

We just flagged the patch with a question, want to figure that out before re-landing.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/838185d7a6e7
Part 1: browser.tabs.onActivated; r=mixedpuppy,rpl,JanH
https://hg.mozilla.org/integration/autoland/rev/e93f7c2b5263
Part 2: expose tab successors in browser.tabs; r=mixedpuppy,rpl

Comment 18

[Narcis Beleuzu [:NarcisB]]

Backed out for 

Backout link: https://hg.mozilla.org/integration/autoland/rev/06be7f6e26ec99e904c705ceb8a84fe2964aab7e
Push link: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception%2Crunnable&selectedJob=212022434&revision=e93f7c2b5263bc5faa3822114dc6b18dca7d2cb9
Log link: https://treeherder.mozilla.org/logviewer.html#?job_id=212022434&repo=autoland&lineNumber=29195

Comment 19

[Ryan Hendrickson [rhendric on GitLab, GitHub]]

<sigh> Okay, that mole was whacked. Sorry for the churn; some of the browser test suites seem to randomly hang for me (even before my changes), or I'd make more of an effort to run all of them. Uno mas?

Comment 20

[Ryan Hendrickson [rhendric on GitLab, GitHub]]

Poke?

Comment 21

[Andrew Swan [:aswan]]

Ryan, we have the try server for running tests in automation before landing:
https://wiki.mozilla.org/ReleaseEngineering/TryServer

I started a run for you with your latest patches:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f02e21f24cc68cdb68f9b3743d2b4ef3593c471b

Comment 22

[Ryan Hendrickson [rhendric on GitLab, GitHub]]

Sure, but I'm just a rando--if I applied for level 1 commit access (in order to submit try jobs in the future), would you or someone else on this bug be able to vouch for me, just on the basis of this interaction? Or is that just for interns/mentees/people with actual relationships with Mozilla?

Comment 23

[Andrew Swan [:aswan]]

There's a lot of overlap between Mozilla employees and committers (and peers etc.) but they are not 1:1.  The governance model is described in great detail on the wiki.  In any case, I'd be happy to vouch for you for level 1 commit access, please needinfo? me on the bug you open(ed) for commit access.

Comment 24

[Ryan Hendrickson [rhendric on GitLab, GitHub]]

I hate to exercise your patience further, but I can't interpret the results of the run you started for me. It looks like no tests were able to run on Android 4.3, but beyond that I can't find any logs pointing to why that is or what I should do about it. All the Treeherder User Guide is able to tell me is that orange means tests failed, and the wiki doesn't seem to have a cheatsheet on what any of this alphabet soup means (I checked the page you linked and the page linked from the Treeherder User Guide). Where does a newbie go to figure out how to be productive with the try server?

Comment 25

[Rob Wu [:robwu]]

The raw log (the second icon on the toolbar at the bottom view when you click on an orange [X1] contains the following near the end:

[task 2018-11-20T02:31:29.155Z] 02:31:29     INFO -  no tests to run using specified combination of filters: skip_if, run_if, fail_if, pathprefix(['browser/components/extensions/test'])
[task 2018-11-20T02:31:29.173Z] 02:31:29    ERROR - Return code: 1
[task 2018-11-20T02:31:29.174Z] 02:31:29    ERROR - No tests run or test summary not found

This means that there is not any test that matches the try syntax:
> try: -b d -p linux64,android-api-16 -u mochitest-1,mochitest-browser-chrome-1,mochitest-browser-chrome-e10s-1,mochitest-e10s-1,mochitest-e10s-browser-chrome-1,xpcshell --try-test-paths browser-chrome:browser/components/extensions/test mochitest:browser/components/extensions/test xpcshell:browser/components/extensions/test --artifact

The try syntax doesn't contain anything that would match the Android test that you have added here (possibly because the try job was generated on desktop).

From comment 18, if you look at the test failure on Android, you can see that it is a TV failure. That means that the new test in your push was run several times under different conditions to see if there are any potential race conditions in the test or implementation.
If you click on [TV], you will see the following at the left of the bottom sidebar:

Job name: test-android-em-4.3-arm7-api-16/debug-test-verify

At https://mozilla-releng.net/trychooser/ you can try to recreate the same task:
- Select debug (let's also do opt just for a good measure)
- Select an Android platform (android api-16+)
- Use artifact build (you didn't edit C++ code)
- Select the test-verify-e10s checkbox

Result: try: -b do -p android-api-16 -u test-verify-e10s -t none --artifact


Once your commit access is activated (bug 1508537), you can try it yourself.
You can also run the test locally with the --verify flag; something like this should work:
mach test mobile/android/components/extensions/test/mochitest/test_ext_tabs_events.html --verify

For instructions to test locally, possibly an emulator, see https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Simple_Firefox_for_Android_build

Comment 26

[Ryan Hendrickson [rhendric on GitLab, GitHub]]

Rob, thanks for your help. Here's the result of running the try command you suggested:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f3f988969d12f22f3b7d17c6d6ee913abf0cb932

The job succeeded, but it doesn't look like any tests actually ran. Is that right? Do I need to select more of the test suites? Is just selecting all of them considered a waste of resources, or is that frequently done?

Comment 27

[Rob Wu [:robwu]]

The TV tests did not run indeed. Since the try syntax isn't helping, you can also just use the web interface of Treeherder instead; especially because you know which tests you'd like to run (from the push in comment 18).

I started the TV tests in your push above; to do it yourself see:
https://wiki.mozilla.org/ReleaseEngineering/TryServer#How_to_push_to_try

If you want to retry a job, and that job has alredy been added once, then to retry you can simply select the job (to show the bottom view) and click on the "Repeat the selected job" button (it is the icon with the curved arrow) in the toolbar of the bottom view.

Comment 28

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

(In reply to ryan.hendrickson from comment #26)
> Rob, thanks for your help. Here's the result of running the try command you
> suggested:
> 
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=f3f988969d12f22f3b7d17c6d6ee913abf0cb932

The above try job does have the TV failures on android debug builds (see https://treeherder.mozilla.org/#/jobs?repo=try&revision=f3f988969d12f22f3b7d17c6d6ee913abf0cb932&selectedJob=213108049)

Based on the errors logged in the failure (e.g. 'Error: Type error for tab value (Unexpected property "successorTabId") for tabs.create.' or 'Error: Type error for result value (Error processing 0: Unexpected property "successorTabId") for tabs.query.'), the reason for these failures should be that we validate the properties of the objects returned/resolved from the API methods using the their related definition in the API schemas (and we do that only in debug builds, e.g. see https://searchfox.org/mozilla-central/rev/b03a62c3c82316e733a3b09622c1cb7e59f64cc3/toolkit/components/extensions/Schemas.jsm#2278-2284).

In D9272 "successorTabId" is added to "browser/components/extensions/schemas/tabs.json", but it doesn't exist yet in the related API schema file used by Fennec (the "mobile/android/components/extensions/schemas/tabs.json" one), nevertheless "successorTabId" is also populated on Fennec (always set to -1 in D9272), and so I expect that the test fails consistently on debug builds because this property in unexpected on Fennec based on the schema.

Adding "successorTabId" definition to "mobile/android/components/extensions/schemas/tabs.json" should be enough to fix that failure.

> The job succeeded, but it doesn't look like any tests actually ran. Is that
> right? Do I need to select more of the test suites? Is just selecting all of
> them considered a waste of resources, or is that frequently done?

Yeah, selecting everything is a huge waste of resources and it is definitely not the solution,
"selecting the right tests in a try job" seems hard and confusing initially, but it also becomes
way easier with some practice (e.g. after a bit you become able to recognize which is the try job
syntax that you need more often).

Comment 29

[Ryan Hendrickson [rhendric on GitLab, GitHub]]

Ah, thank you, that was very helpful! The fact that the schemata are only checked on debug builds was especially good to know; I didn't realize I was testing an optimized build locally, and that was the missing piece I needed to reproduce the test failure locally.

I've updated and rebased the patches once more. My most recent try run is here:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4584c591df12e5b15888c20ef1b583812f950b07

There are two jobs that failed initially, but succeeded on a retry. One of the failures looks like an intermittent failure that is already documented; the other (the TV job) is in a test suite that I touched, but it doesn't look like something I could have caused--of course, I could be mistaken. Should I file a bug for that failure? And is this good enough to try landing the patches once more, or is the TV job cause for more concern than that?

Comment 30

[Rob Wu [:robwu]]

The TV failure seems unrelated to your patch. It looks like there are already some reports of it: bug 1509755 , bug 1498390 , bug 1499621 , bug 1503096

Since another retrigger of the TV has passed, and your original debug-only issue has been resolved, I believe that it is safe to retry landing the patches.

Comment 31

[Ryan Hendrickson [rhendric on GitLab, GitHub]]

Land please?

Comment 32

[Rob Wu [:robwu]]

To land the patch, edit the bug and add the checkin-needed keyword.

Comment 33

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f5bf87ab6448
Part 1: browser.tabs.onActivated; r=mixedpuppy,rpl,JanH

Comment 34

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/cd420001c8ea
Part 2: expose tab successors in browser.tabs; r=mixedpuppy,rpl

Comment 35

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/f5bf87ab6448
https://hg.mozilla.org/mozilla-central/rev/cd420001c8ea

Comment 36

[YUKI "Piro" Hiroshi]

For my understanding, I've wrote effective usages of new APIs, especially about the tabs.moveInSuccession(), with many figures.
https://qiita.com/piroor/items/fb969c94a41c36fd56f5
This article is written in Japanese, so I'll try to translate it to English...

Comment 37

[Ryan Hendrickson [rhendric on GitLab, GitHub]]

I'm not a Japanese speaker, but even the Google-translated version makes it clear that this is a very detailed and carefully written article; well done!

There's one detail about tab successors that I don't think you mentioned (sorry if I missed it; Google's translation was pretty rough), which helps to explain why moveInSuccession is important, besides the performance benefits which you did mention. When Firefox closes a tab, the succession order automatically adjusts around it. So if the succession order is A -> B -> C, and B is closed (or moved to another window), then A's successor will be set to C by Firefox—the add-on doesn't need to manage this.

So even though it seems like you can do the same thing moveInSuccession does with the low-level get and update functions, the asynchronous communication between the Firefox parent process and the add-on makes it theoretically possible that something about the set of tabs will change while the add-on is working on setting up the successor tabs in the desired order. For example, if I'm trying to set the chain A -> B -> C without moveInSuccession, the user (or another add-on) may close B after I've set it as the successor to A, but before I've set C as the successor to B. In that case, A will take as its successor whatever B's original successor was, which is not what I'd want (I want it to be C). moveInSuccession, in contrast, is atomic—it changes all of the successors it's going to change at once, and so any tab events that might affect the succession order will only take effect entirely before or entirely after the moveInSuccession call does its work. In the example above, if B is closed before moveInSuccession is handled, then moveInSuccession will ignore the invalid tab B and just set A -> C. If B is closed after moveInSuccession is handled, then Firefox will adjust the succession order from A -> B -> C to A -> C—so it's the same result either way.

So that's why there is both a low-level way and a high-level way to set tab successors. I left the low-level option in the design for two reasons: moveInSuccession *is* harder to understand, and it's possible there's some use case I haven't thought of where moveInSuccession is insufficient. But I personally use moveInSuccession only, even when just setting one tab's successor, because moveInSuccession closes up the succession order behind the moved tab(s) the same way that Firefox does when a tab is closed, and it does that atomically. And that's what I'd recommend to anyone else manipulating tab successors, unless they find some problem with it.

Comment 38

[YUKI "Piro" Hiroshi]

Thank you for comments, Ryan! I've updated/translated the article to English and published:
https://qiita.com/piroor/items/ea7e727735631c45a366
Mirror: https://piro.sakura.ne.jp/latest/blosxom/mozilla/xul/2018-12-03_successor-tabs-api-en.htm

Comment 39

[Ben Bucksch (:BenB)]

Thunderbird bug 1511838.

Comment 40

[Vlad Jiman]

It is my understanding that automated tests are prepared and can be run, will you also require manual validation? if yes please specify some steps on howto correctly validate, thanks!

Comment 41

[Ryan Hendrickson [rhendric on GitLab, GitHub]]

Vlad, I don't think manual testing is necessary for this; the automated tests cover all the actions and effects I would think of to test. (But if there's a policy encouraging manual testing for this kind of feature anyway, I could write a test plan for you.)

Comment 42

[Vlad Jiman]

The main thing is for the feature to be validated and make sure it works correctly, if this is done through automation and it's comprehensive enough to be marked validated, it's good enough for me, just let me know if you need my assistance on this, I'll flag it as qe-validate- until then.

Comment 43

[Chris Mills [:cmills]]

Note to MDN writers:

I've added a set of notesto the Fx65 rel notes to cover this addition:
https://developer.mozilla.org/en-US/docs/Mozilla/Firefox/Releases/65#API_changes

Hope that covers it!

From those notes, the documentation to add should be fairly self explanatory.

Comment 44

[Irene Smith]

Although I see mentions of moveInSuccessionAfter, I can't find that it's been implemented. Has it? I mean, we mention it in the release notes, but I can't find any other reference to it.

Comment 45

[Dão Gottwald [:dao]]

(In reply to Irene Smith from comment #44)

Although I see mentions of moveInSuccessionAfter, I can't find that it's been implemented. Has it? I mean, we mention it in the release notes, but I can't find any other reference to it.

This was merged into moveInSuccession, see https://phabricator.services.mozilla.com/D9272#inline-39164

Comment 46

[Irene Smith]

Thank you for responding so quickly! Based on your answer, I removed the reference to moveInSuccessionAfter from the release notes

I have:

tabs.Tab - added successorId
tabs.onActivated() - added previousTabId
tabs.update() - added successorTabId
created new page tabs.moveInSuccession()
no page for moveInSuccessionAfter() because the separate method was not created. The behavior was added to moveInSuccession()

I also created a pull request to create BCD for moveInSuccession