microsoft / playwright-java

Java version of the Playwright testing and automation library
https://playwright.dev/java/
Apache License 2.0
1.15k stars 207 forks source link

[BUG] BrowserContext should be aware of pages opened by external sources #461

Closed dakLSS closed 3 years ago

dakLSS commented 3 years ago

BrowserContext.pages() does not know if a page is opened by a source other than Playwright. If an app other than Playwright opens a page, it doesn't get added to context.pages(). This includes the recommended pattern from the docs:

If the action that triggers the new page is unknown, the following pattern can be used:

// Get all new pages (including popups) in the context
context.onPage(page -> {
page.waitForLoadState();
System.out.println(page.title());
});

The event is not emitted if a page is opened by an external source. It is emitted with context.newPage(), however

yury-s commented 3 years ago

Can you upload a code snippet where it reproduces? Also can you elaborate on "a page is opened by an external source" - is it caused by a human interaction with the browser?

In the example above can it be that page.waitForLoadState(); hangs because the page never finishes loading?

dakLSS commented 3 years ago

I can't really provide a snippet because it's a process between a host app and a Chrome extension. The host app sends a link to the client Chrome extension, and the extension opens the link in a new tab. Should be the equivalent of right-clicking a link in a page and selecting 'open link in a new tab', manually. I can try that to see if I get the same behavior.

No, the event is never emitted, so page.waitForLoadState() is never called. If I call context.newPage() the event is emitted for context.onPage(). So the event handler works if the call originates from Playwright, but not from external sources.

What I do to work around it is step through the expected number of tabs with context.pages().get(n) wrapped in a try/catch block. After I do that, a call to context.pages().size() returns the correct value. Before stepping through them, context.pages().size() does not include the tab opened by the app.

yury-s commented 3 years ago

Ok, if there is an extension in the browser it runs in the persistent context. Browser.newContext() will create ephemeral context which will be completely isolated. How do you launch the browser with extension and how do you create the context where you listen for new pages?

It should be something like this:

    BrowserContext context = browserType.launchPersistentContext(Paths.get("/tmp/my-profile"),
      new BrowserType.LaunchPersistentContextOptions().setArgs(asList("--load-extension=<path to the extension>")));

Then if you listen for new pages on that context the event should fire for the tabs opened by the extension.

dakLSS commented 3 years ago

This is what I'm using to launch, exactly as you say, but with a few more options:

public void studentLaunch(String userEmail, int instanceCount) throws Throwable {
        emulateUser(userEmail);
        Path userDataDir = Paths.get("/tmp/chrome-user" + Integer.toString(instanceCount));
        String extensionPath =
                "./src/test/java/classroom/resources/extensions/dest";
        List<String> chromeArgs = new ArrayList<>();
        chromeArgs.add("--disable-background-timer-throttling");
        chromeArgs.add("--disable-backgrounding-occluded-windows");
        chromeArgs.add("--disable-renderer-backgrounding");
        chromeArgs.add("--no-sandbox");
        chromeArgs.add("--disable-setuid-sandbox");
        chromeArgs.add("--disable-dev-shm-usage");
        chromeArgs.add("--enable-background-networking");
        chromeArgs.add("--disable-extensions-except=" + extensionPath);
        chromeArgs.add("--load-extension=" + extensionPath);
        BrowserType.LaunchPersistentContextOptions options = new BrowserType.LaunchPersistentContextOptions();
        options.channel = BrowserChannel.CHROME;
        options.setHeadless(false);
        options.setDevtools(false);
        options.args = chromeArgs;

        studentContext = student.chromium().launchPersistentContext(userDataDir, options);
        studentPage = studentContext.pages().get(0);
        studentPage.setDefaultTimeout(60000);
    }
dakLSS commented 3 years ago

I think I may have a race condition on when the event is emitted, since the listener is asynchronous. I believe it is in fact emitted, but too late in my flow. Guess I need some sort of Promise handler. Not sure how to do that with Java.

So, I essentially add the listener:

studentContext.onPage(studentPage -> {
            studentPage.waitForLoadState();
            System.out.println("Student has " + studentContext.pages().size() + " tabs open");
        });

Then the action that opens the new tab occurs

Then I call studentContext.pages.get(1).title() to verify the student received the tab. But this call is happening before the listener catches the event, and so .get(1) is out of range.

dakLSS commented 3 years ago

I implemented a callback interface, but it's still emitting the event too late. Trying to figure out how to block execution until it's emitted. Seems to me that context.onPage(..) should detect the new tab right away. The listener is in place before the app opens the new tab. Puzzling. It's probably my async flow handling, and not a bug in Playwright, but I'm still trying to determine that for sure.

dakLSS commented 3 years ago

I'm thinking my workaround is simpler. Not elegant, but simpler than implementing context.onPage(). The workaround is just to loop through the expected number of pages with context.pages().get(n).bringToFront(). Once I do that, Playwright is aware of all the pages. context.pages().size() etc. Instinctively, I thought I Playwright should already know about all the pages, since the tab is opened in a Playwright context (at least the BrowserContext), and that I should immediately be able to check context.pages().size() and get the correct number. That's not the case.

yury-s commented 3 years ago

The problem with waiting for an event is that someone has to pump incoming messages from the driver and dispatch them on the playwright thread. You can use waitForPage to achieve that:

    Page newPage = studentContext.waitForPage(() -> {});

Since in your case the page is opened externally, the callback is just a no-op. Internally waitForPage will dispatch incoming messages as soon as they are received (it will also trigger all page listeners on the context too).

If you are curious the reason why context.pages().get(n).bringToFront() and just context.pages().get(n) didn't is that bringToFront() sends a message to the driver and then dispatches all messages from it until the response is received while context.pages() uses local state and doesn't interact with the driver. This is a fundamental flaw of the synchronous API at the moment but luckily most of the time there is interaction over the protocol or one can use waitFor* to ensure that.

dakLSS commented 3 years ago

Yes, that makes sense, and Page newPage = studentContext.waitForPage(() -> {}); does work well. I was thinking it would be difficult to test to make sure multiple students received the new page on a single event (a teacher can send to all students in the class, with our app), but I'm currently testing only 2 students at a time, with separate Playwright instances, so I can set up one waitForPage for each. Thanks very much for your input! I'll leave it to your discretion about this ticket status, of course.

dakLSS commented 3 years ago

Correction: my original thinking was right about not being able to use Page newPage = studentContext.waitForPage(() -> {}); for multiple recipients on a single event, because the event has to be inside the -> {} block. Can you think of a way to do it that I might be missing?

yury-s commented 3 years ago

Something like this should work for multiple playwright instances on the same thread:

    BrowserContext studentContext1 = ...;
    BrowserContext studentContext2 = ...;
    ...
    boolean[] received1 = {false};
    boolean[] received2 = {false};
    studentContext1.onPage(p -> received1[0] = true);
    studentContext2.onPage(p -> received2[0] = true);
    while (!received1[0]) {
      pageInContext1.waitForTimeout(10);
    }
    while (!received2[0]) {
      pageInContext2.waitForTimeout(10);
    }

Basically you'd run a message loop for each instance until the condition is satisfied. Page.waitForTimeout() is somewhat ugly here as it requires having a page. We considered adding something like Playwright.waitForCondition(Predicate) or event Playwright.sleep(timeout) which would provide a method pumping the messages on the top level object which is always available. We are collecting more feedback on how people use it and what their requirements are to shape better API for this. Meanwhile doing this manually with page.waitForTimeout should work.

I assume you don't want to mess with multi-threading. It would be another solution - run each student's playwright instance on its own thread so that blocking one of them doesn't affect the other but. It would unnecessarily over-complicate the code and would require extra synchronization for the cross-thread communication, totally understand that, a single-threaded solution seems much simpler.

dakLSS commented 3 years ago

Thanks very much, Yury. Yes, that would work. You're also right that a multi-threaded approach would be unnecessarily complex. I would only do it if I needed maximum performance with parallel processing. For now, looping through the expected tabs works well, for each student instance.

for (int i = 0; i < numTabs; i++) {
    studentContext.pages().get(i).waitForLoadState();
    studentContext.pages().get(i).bringToFront();
}
yury-s commented 3 years ago

Closing this issue as you seem to have a solution for the problem. Meanwhile, we'll keep collecting feedback on how the events are dispatched to listeners to see what API would work better.

Feel free to reopen if you need more assistance.

dakLSS commented 3 years ago

Thanks again for all the help. Playwright has been a real game changer for me. Makes UI test automation far easier, faster, more reliable, and more maintainable.