rod icon indicating copy to clipboard operation
rod copied to clipboard

Panic in Screenshot

Open yusufozturk opened this issue 2 years ago • 7 comments

Rod Version: v0.107.2

What you got

page.go:371 throws "invalid memory address or nil pointer deference"

image

I think "metrics" or "CSSContentSize" is nil. This happens in a customer environment, so I can't repro the issue when I want. I see the panic from the logs. I will try to do some debug but maybe you already have an idea which one might be nil?

What you expected to see

Instead of panic, it should return error.

What have you tried to solve the question

I can add nil check for metrics and CSSContentSize.

yusufozturk avatar Jun 21 '22 22:06 yusufozturk

Please fix the format of your markdown:

9:244 MD009/no-trailing-spaces Trailing spaces [Expected: 0 or 2; Actual: 1]

generated by check-issue

rod-robot avatar Jun 21 '22 22:06 rod-robot

Can we create a fake HTML page to reproduce this?

ysmood avatar Jun 22 '22 01:06 ysmood

So as I understand, I should create a fake HTML page and PageGetLayoutMetricsResult should return nil for it?

I don't know how to do that yet. But I had 2 ideas:

  1. Maybe customer has old Chrome version so only "ContentSize" filled in and not "CSSContentSize" ? It seems like "ContentSize" is deprecated.
  2. Maybe just before PageGetLayoutMetricsResult request, browser process was killed? That can happen sometimes due to antivirus, 3rd party tools etc. In that case, entire "metrics" will be nil.

yusufozturk avatar Jun 22 '22 01:06 yusufozturk

About 2, it won't panic, because rod has protection for crashing, it will return proper err for it.

About 1, if we can't reproduce it, we won't merge the PR.

ysmood avatar Jun 22 '22 05:06 ysmood

Okay let me test it some more. If I can repro the issue, I can reopen the PR.

Thanks.

yusufozturk avatar Jun 22 '22 20:06 yusufozturk

@ysmood customer had an old Chrome version, so data was actually under ContentSize and CSSContentSize was nil.

We upgraded chrome version in that customer to avoid this issue. But there might be other people who has similar problem. As the package owner, it's up to you if you want to add a nil check or not. I don't know how I can provide a test with an old version of Chrome but at least I can provide you some screenshots of the problematic Chrome.

image

image

PS: Please keep in mind, usually Enterprise has no internet access for internal networks. Chrome installations usually made by system engineers in the beginning of the server installation and no one updates it because nobody uses Chrome in the server and it's not in the patching plan. So it's normal to have old versions in some cases.

yusufozturk avatar Jul 18 '22 22:07 yusufozturk

Sure, we can make a PR for it, let's reopen it. Can you help to make the PR, thank you so much!

ysmood avatar Jul 19 '22 03:07 ysmood