Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Setting permissions are flaky in firefox #720

Closed
JoelEinbinder opened this issue Jan 28, 2020 · 2 comments · Fixed by #1267
Closed

Setting permissions are flaky in firefox #720

JoelEinbinder opened this issue Jan 28, 2020 · 2 comments · Fixed by #1267
Assignees

Comments

@JoelEinbinder
Copy link
Contributor

It looks like the permissions onchange event is fired asynchronously after we set the permission in firefox. This causes the permission.spec.js test "should trigger permission onchange" to be flaky. It flakes for me about 1/1000 times locally, and very frequently on the bots.

@JoelEinbinder
Copy link
Contributor Author

I was debating whether the feature is flaky, or the test needs to be changed. But I would expect Browser.grantPermissions to wait until all pages have received the new permissions.

@JoelEinbinder
Copy link
Contributor Author

Long running test to repro:

      await page.goto(server.EMPTY_PAGE);
      await page.evaluate(() => {
        window['state'] = null;
        return navigator.permissions.query({name: 'geolocation'}).then(function(result) {
          window['state'] = result.state;
          result.onchange = function() {
            window['state'] = result.state;
          };
        });
      });
      for (var i = 0; i < 5000; i++) {
        console.log(i);
        await context.setPermissions(server.EMPTY_PAGE, []);
        expect(await page.evaluate(() => window['state'])).toEqual('denied');
        await context.setPermissions(server.EMPTY_PAGE, ['geolocation']);
        expect(await page.evaluate(() => window['state'])).toEqual('granted');
      }

use TIMEOUT=0

Also it seems to take firefox a long time to close, which is dependent on how long I make the loop. Maybe we have a leak?

@dgozman dgozman added the bug label Feb 27, 2020
@JoelEinbinder JoelEinbinder self-assigned this Feb 28, 2020
aslushnikov added a commit to aslushnikov/playwright that referenced this issue Mar 4, 2020
Refactor inter-process communication inside Firefox. The goal is
to have a single abstraction that works nicely for all our cross-process
communication needs (browser <-> content, content <-> workers, content
<-> file:// process, e.t.c.)

This is step 1 that eliminates content sessions everywhere.
Step 2 will move workers onto `SimpleChannel` as well.

This is a pre-requisite for microsoft#720: with a single `browser <-> content`
communication channel it will be easier to await permission change in tabs.

References microsoft#720
aslushnikov added a commit to aslushnikov/playwright that referenced this issue Mar 4, 2020
Review URL: aslushnikov/juggler@6364381

Refactor inter-process communication inside Firefox. The goal is
to have a single abstraction that works nicely for all our cross-process
communication needs (browser <-> content, content <-> workers, content
<-> file:// process, e.t.c.)

This is step 1 that eliminates content sessions everywhere.
Step 2 will move workers onto `SimpleChannel` as well.

This is a pre-requisite for microsoft#720: with a single `browser <-> content`
communication channel it will be easier to await permission change in tabs.

References microsoft#720
aslushnikov added a commit that referenced this issue Mar 4, 2020
Review URL: aslushnikov/juggler@6364381

Refactor inter-process communication inside Firefox. The goal is
to have a single abstraction that works nicely for all our cross-process
communication needs (browser <-> content, content <-> workers, content
<-> file:// process, e.t.c.)

This is step 1 that eliminates content sessions everywhere.
Step 2 will move workers onto `SimpleChannel` as well.

This is a pre-requisite for #720: with a single `browser <-> content`
communication channel it will be easier to await permission change in tabs.

References #720
aslushnikov added a commit to aslushnikov/playwright that referenced this issue Mar 5, 2020
Review URL: aslushnikov/juggler@ff985e0

Wait for permissions to propagate to all context pages.

Fixes microsoft#720
aslushnikov added a commit to aslushnikov/playwright that referenced this issue Mar 5, 2020
Review URL: aslushnikov/juggler@9bd6e72

Wait for permissions to propagate to all context pages.

Fixes microsoft#720
aslushnikov added a commit that referenced this issue Mar 5, 2020
Review URL: aslushnikov/juggler@9bd6e72

Wait for permissions to propagate to all context pages.

References #720
aslushnikov added a commit to aslushnikov/playwright that referenced this issue Mar 6, 2020
aslushnikov added a commit that referenced this issue Mar 6, 2020
…als (#1267)

Drive-by: fixes #720 since it rolls past r1037. 

Co-authored-by: Dmitry Gozman <dgozman@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment