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

Screenshotter fails after a JavaScript exception #1502

Open
5 tasks
inancgumus opened this issue Oct 31, 2024 · 12 comments
Open
5 tasks

Screenshotter fails after a JavaScript exception #1502

inancgumus opened this issue Oct 31, 2024 · 12 comments
Assignees
Labels
bug Something isn't working

Comments

@inancgumus
Copy link
Member

inancgumus commented Oct 31, 2024

What?

The screenshotter fails when called in a catch block (after an exception).

export async function someTest() {
  ...
  try {
    ...
  } catch (error) {
    await page.screenshot(...);
  } finally {
    await page.close();
  }

Exception:

panic: runtime error: invalid memory address or nil pointer dereference [recovered]
panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x30 pc=0x15e1f48]

goroutine 11479469 [running]:
go.opentelemetry.io/otel/sdk/trace.(*recordingSpan).End.deferwrap1()
go.opentelemetry.io/otel/[email protected]/trace/span.go:398 +0x25
go.opentelemetry.io/otel/sdk/trace.(*recordingSpan).End(0xc00ff77d40, {0x0, 0x0, 0xc0615c1a40?})
go.opentelemetry.io/otel/[email protected]/trace/span.go:436 +0xa62
panic({0x197cc60?, 0x2f63d50?})
runtime/panic.go:785 +0x132
github.com/grafana/xk6-browser/common.(*screenshotter).screenshot(0xc017fa5ee0, {0x1f9b8c0, 0xc01df2c780}, 0x0, 0xc017fa5cd8, {0x1c15435, 0x3}, 0x0, 0xc04cc80d80?, {0xc09641fa10, ...})
github.com/grafana/[email protected]/common/screenshotter.go:185 +0x328
github.com/grafana/xk6-browser/common.(*screenshotter).screenshotPage(0xc017fa5ee0, 0xc03747c680, 0xc04cc80ac0)
github.com/grafana/[email protected]/common/screenshotter.go:398 +0x334
github.com/grafana/xk6-browser/common.(*Page).Screenshot(0xc03747c680, 0xc04cc80ac0, {0x7f691c1821d0, 0xc000451d10})
github.com/grafana/[email protected]/common/page.go:1356 +0x2c6
github.com/grafana/xk6-browser/browser.mapPage.func33.1()
github.com/grafana/[email protected]/browser/page_mapping.go:241 +0x4e
github.com/grafana/xk6-browser/k6ext.promise.func1()
github.com/grafana/[email protected]/k6ext/promise.go:24 +0x2c
created by github.com/grafana/xk6-browser/k6ext.promise in goroutine 730
github.com/grafana/[email protected]/k6ext/promise.go:23 +0x9a

Test Run ID: 3439289

Why?

Users should take screenshots even after an exception to see the issues in their web pages or scripts.

How?

Looking at the code:

  • screenshotPage calls screenshotter.screenshot with a non-nil viewport *Rect.
  • However, the code panics here while calculating Width: viewport.Width / visualViewport.Scale.

Since viewport should not be nil, visualViewport might be.

However, the signature in the panic log says that viewport is nil:

github.com/grafana/xk6-browser/common.(*screenshotter).screenshot(0xc017fa5ee0, {0x1f9b8c0, 0xc01df2c780}, 0x0, 0xc017fa5cd8, {0x1c15435, 0x3}, 0x0, 0xc04cc80d80?, {0xc09641fa10, ...})

According to the code, the screenshotter.go:398 seems not to pass a nil Rect, and always passes a non-nil one (here and here) unless there's an error.

Tasks

Tasks

Related PR(s)/Issue(s)

No response

@inancgumus inancgumus added the bug Something isn't working label Oct 31, 2024
@ankur22
Copy link
Collaborator

ankur22 commented Nov 1, 2024

This could be linked to #1056. clickablePoint works with cdppage.GetLayoutMetrics which is the same CDP request performed before the NPD in this issue.

@ankur22
Copy link
Collaborator

ankur22 commented Nov 8, 2024

The first argument in the stack trace (0xc017fa5ee0) is the receiver, which is a pointer to the screenshotter instance, so viewport is not nil since it is 0xc017fa5cd8, but doc is nil which matches the call from github.com/grafana/[email protected]/common/screenshotter.go:398. So my hypothesis is likely to be correct -- avoid working with cdppage.GetLayoutMetrics, and no more NPDs.

@yevk
Copy link

yevk commented Dec 5, 2024

I'm experiencing the same exact issue. Had to revert to 0.53.0 for screenshots to work on error.

@olegbespalov
Copy link
Contributor

olegbespalov commented Dec 11, 2024

Hi @yevk !

Had to revert to 0.53.0 for screenshots to work on error.

Do you mean that you see no such issues in k6 v0.53? 🤔

UPD: also, do you maybe have a minimal script to reproduce the issue?

@yevk
Copy link

yevk commented Dec 11, 2024

That's right, 0.54 produced the same error but 0.53 did not. I'm taking screenshots only when an error occurs.

I don't have a minimal script at the moment, I only have a full script which I can't share.
I'll try to produce one, but I can't promise I'll have it soon due to time constraints.

@olegbespalov
Copy link
Contributor

That's right, 0.54 produced the same error but 0.53 did not. I'm taking screenshots only when an error occurs.

That's interesting! 🤔

Would be feasible for you to validate if the fix that we're trying to bring work locally?

@yevk
Copy link

yevk commented Dec 11, 2024

Sure. Just let me know how to install it.

@olegbespalov
Copy link
Contributor

olegbespalov commented Dec 11, 2024

Thanks, that will be helpful! 🙇

Okay, for that, you have to have on your machine the xk6, the custom k6 builder

And build the custom version of the k6 (including the update from #1559) which should workaround the issue (at least not produce panic, but warning)

xk6 build --output xk6-browser --with github.com/grafana/xk6-browser@6fe54282b296e2ec1bd6209068333c547824bb4f

That should produce a custom k6 (with the name xk6-browser) in the catalog where the command was executed, and you could run using local binary by

./xk6-browser run script.js

If that's not feasible, that's okay, I'll still think of the way how I can reproduce it locally

Note: I've updated xk6 build command to reflect the current latest commit 6fe54282b296e2ec1bd6209068333c547824bb4f

@olegbespalov
Copy link
Contributor

For the record. We merged #1559 which should prevent panic and hopefully, in the worst-case produce a warning log, asking to provide reproducible details to this issue (that's why I haven't closed it yet).

My concern is that since it works in k6 v0.53, and it does not in k6 v0.53, that sounds like a big red flag that it's not related to Chrome (I'm assuming that that installation didn't change between versions). Also, I don't see any update of the between the versions browser versions pinned in that releases, which could signals that issue was introduced somewhere other place.

diff --git a/go.mod b/go.mod
index 80c89c14..63c336f5 100644
--- a/go.mod
+++ b/go.mod
@@ -1,44 +1,45 @@
 module github.com/grafana/xk6-browser
 
-go 1.20
+go 1.21
 
 require (
 	github.com/chromedp/cdproto v0.0.0-20221023212508-67ada9507fb2
 	github.com/gorilla/websocket v1.5.1
-	github.com/grafana/sobek v0.0.0-20240613124309-cb36746e8fee
+	github.com/grafana/sobek v0.0.0-20240829081756-447e8c611945
 	github.com/mailru/easyjson v0.7.7
 	github.com/mccutchen/go-httpbin v1.1.2-0.20190116014521-c5cb2f4802fa
 	github.com/mstoykov/k6-taskqueue-lib v0.1.0
 	github.com/sirupsen/logrus v1.9.3
 	github.com/stretchr/testify v1.9.0
-	go.k6.io/k6 v0.52.0
-	go.opentelemetry.io/otel v1.24.0
-	go.opentelemetry.io/otel/trace v1.24.0
-	golang.org/x/net v0.26.0
-	golang.org/x/sync v0.7.0
+	go.k6.io/k6 v0.53.1-0.20240925100229-86ab6e3ceee8
+	go.opentelemetry.io/otel v1.29.0
+	go.opentelemetry.io/otel/trace v1.29.0
+	golang.org/x/net v0.28.0
+	golang.org/x/sync v0.8.0
 	gopkg.in/guregu/null.v3 v3.3.0
 )

Still, good to have #1559 since we stopped panic and migrate from deprecated field, but looking for a reproducible example

@olegbespalov
Copy link
Contributor

Hi @yevk !

Since we merged the changes, and also updated the version of the browser in k6, we could try if the fix helps without using xk6.

For that, you could install the version of the k6 (golang is still required) from master:

go install go.k6.io/k6@master

If you still have capacity, could you please validate if the fix works or if it at least doesn't cause panic. Thanks!

@yevk
Copy link

yevk commented Dec 13, 2024

Hey @olegbespalov, sorry for the delay and thanks for the update!

I'll try it this weekend and get back to you.

@yevk
Copy link

yevk commented Dec 14, 2024

@olegbespalov

I was able to reproduce the panic with 0.54 and 0.55 (unpatched) a few times.
I could not reproduce even once using the patched version. 👏🏻

I'll keep using the patched version and update you if it panics again. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

4 participants