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

Limit browser implementation to hold a single browserContext #929

Merged
merged 6 commits into from
Jun 14, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 21 additions & 32 deletions common/browser.go
Original file line number Diff line number Diff line change
Expand Up @@ -50,8 +50,7 @@ type Browser struct {
// A *Connection is saved to this field, see: connect().
conn connection

contextsMu sync.RWMutex
Copy link
Collaborator Author

@ankur22 ankur22 Jun 12, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've done a stress test on this mutex removal change, and it all seems to work still.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😱🤞🙃

contexts map[cdp.BrowserContextID]*BrowserContext
context *BrowserContext
defaultContext *BrowserContext

// Cancel function to stop event listening
Expand Down Expand Up @@ -103,7 +102,6 @@ func newBrowser(
state: int64(BrowserStateOpen),
browserProc: browserProc,
browserOpts: browserOpts,
contexts: make(map[cdp.BrowserContextID]*BrowserContext),
pages: make(map[target.ID]*Page),
sessionIDtoTargetID: make(map[target.SessionID]target.ID),
vu: k6ext.GetVU(ctx),
Expand Down Expand Up @@ -137,23 +135,19 @@ func (b *Browser) disposeContext(id cdp.BrowserContextID) error {
return fmt.Errorf("disposing browser context ID %s: %w", id, err)
}

b.contextsMu.Lock()
defer b.contextsMu.Unlock()
delete(b.contexts, id)
b.context = nil

return nil
}

// getDefaultBrowserContextOrByID returns the BrowserContext for the given page ID.
// getDefaultBrowserContextOrMatchedID returns the BrowserContext for the given browser context ID.
// If the browser context is not found, the default BrowserContext is returned.
func (b *Browser) getDefaultBrowserContextOrByID(id cdp.BrowserContextID) *BrowserContext {
b.contextsMu.RLock()
defer b.contextsMu.RUnlock()
browserCtx := b.defaultContext
if bctx, ok := b.contexts[id]; ok {
browserCtx = bctx
}
return browserCtx
func (b *Browser) getDefaultBrowserContextOrMatchedID(id cdp.BrowserContextID) *BrowserContext {
if b.context == nil || b.context.id != id {
return b.defaultContext
}

return b.context
}

func (b *Browser) getPages() []*Page {
Expand Down Expand Up @@ -227,7 +221,7 @@ func (b *Browser) onAttachedToTarget(ev *target.EventAttachedToTarget) {

var (
targetPage = ev.TargetInfo
browserCtx = b.getDefaultBrowserContextOrByID(targetPage.BrowserContextID)
browserCtx = b.getDefaultBrowserContextOrMatchedID(targetPage.BrowserContextID)
)

if !b.isAttachedPageValid(ev, browserCtx) {
Expand Down Expand Up @@ -377,11 +371,8 @@ func (b *Browser) onDetachedFromTarget(ev *target.EventDetachedFromTarget) {
}

func (b *Browser) newPageInContext(id cdp.BrowserContextID) (*Page, error) {
b.contextsMu.RLock()
browserCtx, ok := b.contexts[id]
b.contextsMu.RUnlock()
if !ok {
return nil, fmt.Errorf("missing browser context: %s", id)
if b.context == nil || b.context.id != id {
return nil, fmt.Errorf("missing browser context %s, current context is %s", id, b.context.id)
}

ctx, cancel := context.WithTimeout(b.ctx, b.browserOpts.Timeout)
Expand All @@ -393,7 +384,7 @@ func (b *Browser) newPageInContext(id cdp.BrowserContextID) (*Page, error) {

waitForPage, removeEventHandler := createWaitForEventHandler(
ctx,
browserCtx, // browser context will emit the following event:
b.context, // browser context will emit the following event:
[]string{EventBrowserContextPage},
func(e any) bool {
tid := <-targetID
Expand Down Expand Up @@ -494,15 +485,11 @@ func (b *Browser) Close() {

// Contexts returns list of browser contexts.
func (b *Browser) Contexts() []api.BrowserContext {
b.contextsMu.RLock()
defer b.contextsMu.RUnlock()

contexts := make([]api.BrowserContext, 0, len(b.contexts))
for _, b := range b.contexts {
contexts = append(contexts, b)
if b.context == nil {
return []api.BrowserContext{}
}

return contexts
return []api.BrowserContext{b.context}
}

// IsConnected returns whether the WebSocket connection to the browser process
Expand All @@ -513,6 +500,10 @@ func (b *Browser) IsConnected() bool {

// NewContext creates a new incognito-like browser context.
func (b *Browser) NewContext(opts goja.Value) (api.BrowserContext, error) {
if b.context != nil {
return nil, errors.New("existing browser context must be closed before creating a new one")
}

action := target.CreateBrowserContext().WithDisposeOnDetach(true)
browserContextID, err := action.Do(cdp.WithExecutor(b.ctx, b.conn))
b.logger.Debugf("Browser:NewContext", "bctxid:%v", browserContextID)
Expand All @@ -525,13 +516,11 @@ func (b *Browser) NewContext(opts goja.Value) (api.BrowserContext, error) {
k6ext.Panic(b.ctx, "parsing newContext options: %w", err)
}

b.contextsMu.Lock()
defer b.contextsMu.Unlock()
browserCtx, err := NewBrowserContext(b.ctx, b, browserContextID, browserCtxOpts, b.logger)
if err != nil {
return nil, fmt.Errorf("new context: %w", err)
}
b.contexts[browserContextID] = browserCtx
b.context = browserCtx

return browserCtx, nil
}
Expand Down
4 changes: 2 additions & 2 deletions common/browser_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -31,11 +31,11 @@ func TestBrowserNewPageInContext(t *testing.T) {
var err error
vu := k6test.NewVU(t)
ctx = k6ext.WithVU(ctx, vu)
b.contexts[id], err = NewBrowserContext(ctx, b, id, nil, nil)
b.context, err = NewBrowserContext(ctx, b, id, nil, nil)
require.NoError(t, err)
return &testCase{
b: b,
bc: b.contexts[id],
bc: b.context,
}
}

Expand Down
44 changes: 35 additions & 9 deletions tests/browser_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,22 +22,48 @@ import (

func TestBrowserNewPage(t *testing.T) {
b := newTestBrowser(t)
p := b.NewPage(nil)
p1 := b.NewPage(nil)
l := len(b.Contexts())
assert.Equal(t, 1, l, "expected there to be 1 browser context, but found %d", l)

p2 := b.NewPage(nil)
l = len(b.Contexts())
assert.Equal(t, 2, l, "expected there to be 2 browser context, but found %d", l)
_, err := b.Browser.NewPage(nil)
assert.EqualError(t, err, "new page: existing browser context must be closed before creating a new one")

err := p.Close(nil)
err = p1.Close(nil)
require.NoError(t, err)
l = len(b.Contexts())
assert.Equal(t, 2, l, "expected there to be 2 browser contexts after first page close, but found %d", l)
err = p2.Close(nil)
require.NoError(t, err)
assert.Equal(t, 1, l, "expected there to be 1 browser context, but found %d", l)

_, err = b.Browser.NewPage(nil)
assert.EqualError(t, err, "new page: existing browser context must be closed before creating a new one")

b.Contexts()[0].Close()
l = len(b.Contexts())
assert.Equal(t, 0, l, "expected there to be 0 browser context, but found %d", l)

_ = b.NewPage(nil)
l = len(b.Contexts())
assert.Equal(t, 2, l, "expected there to be 2 browser contexts after second page close, but found %d", l)
assert.Equal(t, 1, l, "expected there to be 1 browser context, but found %d", l)
}

func TestBrowserNewContext(t *testing.T) {
b := newTestBrowser(t)
bc1, err := b.NewContext(nil)
assert.NoError(t, err)
l := len(b.Contexts())
assert.Equal(t, 1, l, "expected there to be 1 browser context, but found %d", l)

_, err = b.NewContext(nil)
assert.EqualError(t, err, "existing browser context must be closed before creating a new one")

bc1.Close()
l = len(b.Contexts())
assert.Equal(t, 0, l, "expected there to be 0 browser context, but found %d", l)

_, err = b.NewContext(nil)
assert.NoError(t, err)
l = len(b.Contexts())
assert.Equal(t, 1, l, "expected there to be 1 browser context, but found %d", l)
}

func TestTmpDirCleanup(t *testing.T) {
Expand Down
11 changes: 9 additions & 2 deletions tests/keyboard_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,8 @@ import (
)

func TestKeyboardPress(t *testing.T) {
tb := newTestBrowser(t)

t.Run("all_keys", func(t *testing.T) {
tb := newTestBrowser(t)
inancgumus marked this conversation as resolved.
Show resolved Hide resolved
p := tb.NewPage(nil)
kb := p.GetKeyboard()
layout := keyboardlayout.GetKeyboardLayout("us")
Expand All @@ -27,6 +26,7 @@ func TestKeyboardPress(t *testing.T) {
})

t.Run("backspace", func(t *testing.T) {
tb := newTestBrowser(t)
p := tb.NewPage(nil)
kb := p.GetKeyboard()

Expand All @@ -43,6 +43,7 @@ func TestKeyboardPress(t *testing.T) {
})

t.Run("combo", func(t *testing.T) {
tb := newTestBrowser(t)
p := tb.NewPage(nil)
kb := p.GetKeyboard()

Expand All @@ -68,6 +69,7 @@ func TestKeyboardPress(t *testing.T) {

t.Run("meta", func(t *testing.T) {
t.Skip("FIXME") // See https://github.com/grafana/xk6-browser/issues/424
tb := newTestBrowser(t)
p := tb.NewPage(nil)
kb := p.GetKeyboard()

Expand All @@ -92,6 +94,7 @@ func TestKeyboardPress(t *testing.T) {
})

t.Run("type does not split on +", func(t *testing.T) {
tb := newTestBrowser(t)
p := tb.NewPage(nil)
kb := p.GetKeyboard()

Expand All @@ -105,6 +108,7 @@ func TestKeyboardPress(t *testing.T) {
})

t.Run("capitalization", func(t *testing.T) {
tb := newTestBrowser(t)
p := tb.NewPage(nil)
kb := p.GetKeyboard()

Expand All @@ -130,6 +134,7 @@ func TestKeyboardPress(t *testing.T) {
})

t.Run("type not affected by shift", func(t *testing.T) {
tb := newTestBrowser(t)
p := tb.NewPage(nil)
kb := p.GetKeyboard()

Expand All @@ -146,6 +151,7 @@ func TestKeyboardPress(t *testing.T) {
})

t.Run("newline", func(t *testing.T) {
tb := newTestBrowser(t)
p := tb.NewPage(nil)
kb := p.GetKeyboard()

Expand All @@ -163,6 +169,7 @@ func TestKeyboardPress(t *testing.T) {

// Replicates the test from https://playwright.dev/docs/api/class-keyboard
t.Run("selection", func(t *testing.T) {
tb := newTestBrowser(t)
p := tb.NewPage(nil)
kb := p.GetKeyboard()

Expand Down
3 changes: 1 addition & 2 deletions tests/network_manager_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -74,11 +74,10 @@ func TestBasicAuth(t *testing.T) {
validPassword = "validpass"
)

browser := newTestBrowser(t, withHTTPServer())

auth := func(tb testing.TB, user, pass string) api.Response {
tb.Helper()

browser := newTestBrowser(t, withHTTPServer())
bc, err := browser.NewContext(
browser.toGojaValue(struct {
HttpCredentials *common.Credentials `js:"httpCredentials"` //nolint:revive
Expand Down