Details
- Reviewers
Sasha - Group Reviewers
webdriver-reviewers-rotation - Commits
- rMOZILLACENTRALdbe6eebd6aff: Bug 1841010 - [remote] Add a NavigationManager to monitor navigations using…
- Bugzilla Bug ID
- 1841010
Diff Detail
- Repository
- rMOZILLACENTRAL mozilla-central
- Build Status
Buildable 561375 Build 658917: arc lint + arc unit
Event Timeline
Hopefully fixed all the issues I spotted on try for the current version: https://treeherder.mozilla.org/jobs?repo=try&revision=519713515afd89d8899e88adb7566bc9e6c70656
| remote/shared/listeners/navigation/NavigationListenerChild.sys.mjs | ||
|---|---|---|
| 35 ↗ | (On Diff #738468) | I triggered awsy jobs and will report back when I have the data. Re events: For now I only use DOMWindowCreated. Maybe later we could use another event triggered before navigation to avoid initializing all actors from the parent process when we start monitoring navigations. beforeunload seems too late. In any case, I'm fine with switching to actorCreated it doesn't change much, except I'll need to keep an empty handleEvent method otherwise we seem to get errors |
Need to stop using singleton and lazy load
| remote/shared/listeners/navigation/NavigationListenerChild.sys.mjs | ||
|---|---|---|
| 35 ↗ | (On Diff #738468) | Unsurprisingly, we see more memory allocated for JS as we are loading an additional actor. One option is to lazily start monitoring navigations. I was reluctant to do that because there are several spots where we might need to have navigation ids available:
But maybe we could simply let those 2 (root) modules call NavigationManager.startMonitoring when they start. I might need to stop using a singleton, it would be risky and hard to cover with tests. So I will probably switch to another approach, where we have an internal singleton state in the NavigationManager to store unique navigation ids, but each consumer still has to get its own instance of NavigationManager. This should be easier to test and maintain. |
| remote/shared/listeners/navigation/NavigationListenerChild.sys.mjs | ||
|---|---|---|
| 35 ↗ | (On Diff #738468) | The numbers from awsy are pretty low so I doubt that this would be a problem and we could keep it. It's basically the cost of new features supported. So I don't have a problem with keeping the singleton and enabling it when the WebDriver session gets started. That clearly allows us to have a good / better initial state of all possibly ongoing navigations. |
| remote/jar.mn | ||
|---|---|---|
| 45 | What if we would actually integrate the navigation listener into the message handler so that all protocols could directly make use of it? Otherwise we have actors here and thinking more about it I don't feel that great in having them added under listeners. I think that we should have them under remote/shared/js-window-actors? | |
| remote/shared/NavigationManager.sys.mjs | ||
| 36–37 | ||
| 104 |
Would indeed be good to know. But feel free to do that in a follow-up bug. | |
| 145 | I assume that this is all done now? | |
| 210 | Please add a comment to clarify that this is used as a singleton. | |
| remote/shared/listeners/navigation/NavigationListenerChild.sys.mjs | ||
| 79 ↗ | (On Diff #739273) | This is not used but only logged. Do we really need it? |
| 97 ↗ | (On Diff #739273) | Would return early be an option here? |
| remote/shared/test/browser/browser_NavigationManager.js | ||
| 22 | Please give this test a name. | |
| 34 | Do you think that we should get rid of getIdForBrowser() and only use getIdForBrowsingContext? That would actually give a clear pattern. If yes, please file a new bug which can be mentored. | |
| 76–78 | If a check fails above we wouldn't clean-up the test correctly. With the clean up function we should at least remove the tab. | |
| 94 | This applies to all the following usages of getIdForBrowser as well. | |
| 134 | ||
| 141 | ||
| 246 | What is the reason why we need this test? The problem with about:blank is basically the first sync load in a new tab, but not when navigating to it. And a remoteness change we already covered in the coop test. Instead I would see a benefit by navigating to an error page so that we cover cases when the navigation stops with DOMContentLoaded. An unusual load might be also good to cover. | |
| 323–339 | Maybe you can move those helpers into the head.js file? | |
So I've more or less scanned this and it seems reasonable to me, but I am less familiar with the code than whimboo or sasha. I think there are still a few comments open from whimboo, but I don't know if you want to wait for him to return, or get sasha to take a look, or just trust that it's had enough review at this point that it's probably going to make the code better, not worse :)
| remote/shared/NavigationManager.sys.mjs | ||
|---|---|---|
| 43 | Hmm, so hypothetically how much would it change if we altered the requirement that the navigation id is unique across sessions? I don't think the current setup was fully thought through from the point of view of multiple sessions, and whilst I don't think it's obviously bad for the ids to be unique for all sessions, I also don't think it's obviously an actual requirement. | |
Thanks for taking a look! I need to come back to whimboo's comments and address them, I'll update this morning. I think I'll stick to a non-singleton API, because it feels more flexible overall. Since I'm instantiating it on the RootMessageHandler as requested, it's very similar to having an always available singleton with the added benefit of having a clearer lifecycle.
I think if Sasha is up for finishing the review, we had enough discussions on it with Henrik to land this version, and see what kind of fixes we can get for puppeteer tests with this. We can still come back to this in follow up bugs if necessary. The main blocker was me who thought I had an issue with same-document navigations, before realizing this was not in scope for this helper :/
Planning changes to address the remaining comments.
| remote/shared/NavigationManager.sys.mjs | ||
|---|---|---|
| 43 | Oh that would be relatively easy to change. Since the navigation id is from the HTML spec at the moment and not a WebDriver BiDi concept, I tried to make sure we would have a really unique id, but I admit I don't have a scenario in mind where it would be crucial to have this identical across sessions... Maybe if several 3rd party tools wanted to interact together somehow, but again I don't have a clear scenario. I can rephrase the comment to make it look less like a requirement and just explain what are the responsibilities of the singleton. | |
| remote/jar.mn | ||
|---|---|---|
| 45 | I am now instantiating a NavigationManager in the RootMessageHandler, so that it's easy to use for all protocols. I can move the actors, but I guess we should have a follow up in order to move all actors to the same folder then? | |
| remote/shared/NavigationManager.sys.mjs | ||
| 145 | yes, added tests as well. | |
| remote/shared/listeners/navigation/NavigationListenerChild.sys.mjs | ||
| 79 ↗ | (On Diff #739273) | It was useful for me to debug, I can guard this behind a check if we really want to avoid creating this. |
| 97 ↗ | (On Diff #739273) | Sure, I was hesitant between the two options. |
| 35 ↗ | (On Diff #738468) | For the record I'm keeping instances, but directly instantiating them on the root message handler. |
| remote/shared/test/browser/browser_NavigationManager.js | ||
| 34 | I would love to, but we already tried and it doesn't seem to be possible: https://bugzilla.mozilla.org/show_bug.cgi?id=1761443#c3 | |
| 76–78 | As you may know, we don't have a way to define a per-task cleanup function (https://bugzilla.mozilla.org/show_bug.cgi?id=1656557). So if we use registerCleanupFunction, it means we will retain the tabs between tasks and will only cleanup at the end. Otherwise it would mean adding try catches everywhere, which is not great for test readability. I will add a addTab helper in head.js here to automatically register the cleanup function for each tab. Again, we'll be retaining tabs between tasks, but I think it's a decent compromise to avoid the try/catches. | |
| 134 | good catch, thanks! | |
It looks like at the moment we don't cover here the case of fragment navigation.
If we run browsingContext.navigate with example.com and then run example.com#foo we receive the response with the same navigationId, which seems like shouldn't be the case according to the spec.
So I guess listening to only state change notifications is not enough, and we should also listen to location change.
| remote/shared/NavigationManager.sys.mjs | ||
|---|---|---|
| 131 | ||
| remote/shared/test/browser/head.js | ||
| 131 | I guess you could move this sentence to the description of the return value. | |
Good point, I was hoping fragmentNavigations would only be observable from the dedicated event, but indeed in case they are triggered from a browsingContext.navigate we still need to be able to have the same navigation id.
Ok so I tried to listen to LOCATION_CHANGE_HASHCHANGE explicitly (and not all location changes, otherwise we started getting navigations for document.write usage, which we don't want in theory).
But we might have to change the way we handle that depending on how we implement fragmentNavigated, because we will really need to make sure fragmentNavigated sees the same id. One easy solution would be to rely on the NavigationManager to implement fragmentNavigated I guess?
| remote/jar.mn | ||
|---|---|---|
| 45 | Yes, please file a new bug. It would really be good to have all the message handler actors in a single spot. | |
| remote/shared/test/browser/browser_NavigationManager.js | ||
| 76–78 |
Oh darn. The per task register cleanup only works for xpcshell tests. I mixed that up. | |