From a3647fea8a01b4a62bd1b0c5ca38f4866ab9c68e Mon Sep 17 00:00:00 2001 From: Julien Neuhart Date: Sat, 7 Sep 2024 15:54:01 +0200 Subject: [PATCH] fix(chromium): increase the scope of loading failed errors (#962) --- pkg/modules/chromium/browser.go | 22 ++++++++------ pkg/modules/chromium/browser_test.go | 8 ++--- pkg/modules/chromium/chromium.go | 4 +-- pkg/modules/chromium/events.go | 45 +++++++++++++++++++++------- pkg/modules/chromium/routes.go | 4 +-- pkg/modules/chromium/routes_test.go | 8 ++--- 6 files changed, 60 insertions(+), 31 deletions(-) diff --git a/pkg/modules/chromium/browser.go b/pkg/modules/chromium/browser.go index 471efcdb..e38c83ba 100644 --- a/pkg/modules/chromium/browser.go +++ b/pkg/modules/chromium/browser.go @@ -315,12 +315,14 @@ func (b *chromiumBrowser) do(ctx context.Context, logger *zap.Logger, url string } var ( - connectionRefused error - connectionRefusedMu sync.RWMutex + loadingFailed error + loadingFailedMu sync.RWMutex ) - // See https://github.com/gotenberg/gotenberg/issues/913. - listenForEventLoadingFailedOnConnectionRefused(taskCtx, logger, &connectionRefused, &connectionRefusedMu) + // See: + // https://github.com/gotenberg/gotenberg/issues/913. + // https://github.com/gotenberg/gotenberg/issues/959. + listenForEventLoadingFailed(taskCtx, logger, &loadingFailed, &loadingFailedMu) err = chromedp.Run(taskCtx, tasks...) if err != nil { @@ -357,12 +359,14 @@ func (b *chromiumBrowser) do(ctx context.Context, logger *zap.Logger, url string return fmt.Errorf("%v: %w", consoleExceptions, ErrConsoleExceptions) } - // See https://github.com/gotenberg/gotenberg/issues/913. - connectionRefusedMu.RLock() - defer connectionRefusedMu.RUnlock() + // See: + // https://github.com/gotenberg/gotenberg/issues/913. + // https://github.com/gotenberg/gotenberg/issues/959. + loadingFailedMu.RLock() + defer loadingFailedMu.RUnlock() - if connectionRefused != nil { - return fmt.Errorf("%v: %w", connectionRefused, ErrConnectionRefused) + if loadingFailed != nil { + return fmt.Errorf("%v: %w", loadingFailed, ErrLoadingFailed) } return nil diff --git a/pkg/modules/chromium/browser_test.go b/pkg/modules/chromium/browser_test.go index 76764b71..d1c54a44 100644 --- a/pkg/modules/chromium/browser_test.go +++ b/pkg/modules/chromium/browser_test.go @@ -480,7 +480,7 @@ func TestChromiumBrowser_pdf(t *testing.T) { expectedError: ErrConsoleExceptions, }, { - scenario: "ErrConnectionRefused", + scenario: "ErrLoadingFailed", browser: newChromiumBrowser( browserArguments{ binPath: os.Getenv("CHROMIUM_BIN_PATH"), @@ -503,7 +503,7 @@ func TestChromiumBrowser_pdf(t *testing.T) { noDeadline: false, start: true, expectError: true, - expectedError: ErrConnectionRefused, + expectedError: ErrLoadingFailed, }, { scenario: "clear cache", @@ -1553,7 +1553,7 @@ func TestChromiumBrowser_screenshot(t *testing.T) { expectedError: ErrConsoleExceptions, }, { - scenario: "ErrConnectionRefused", + scenario: "ErrLoadingFailed", browser: newChromiumBrowser( browserArguments{ binPath: os.Getenv("CHROMIUM_BIN_PATH"), @@ -1577,7 +1577,7 @@ func TestChromiumBrowser_screenshot(t *testing.T) { noDeadline: false, start: true, expectError: true, - expectedError: ErrConnectionRefused, + expectedError: ErrLoadingFailed, }, { scenario: "clear cache", diff --git a/pkg/modules/chromium/chromium.go b/pkg/modules/chromium/chromium.go index 8e3806e6..e5f8f408 100644 --- a/pkg/modules/chromium/chromium.go +++ b/pkg/modules/chromium/chromium.go @@ -42,8 +42,8 @@ var ( // is set to true. ErrConsoleExceptions = errors.New("console exceptions") - // ErrConnectionRefused happens when a URL cannot be reached. - ErrConnectionRefused = errors.New("connection refused") + // ErrLoadingFailed happens when a URL failed to load. + ErrLoadingFailed = errors.New("loading failed") // PDF specific. diff --git a/pkg/modules/chromium/events.go b/pkg/modules/chromium/events.go index f43b2c89..8a8ca1dd 100644 --- a/pkg/modules/chromium/events.go +++ b/pkg/modules/chromium/events.go @@ -21,7 +21,8 @@ import ( ) // listenForEventRequestPaused listens for requests to check if they are -// allowed or not. +// allowed or not.network.SetBlockedURLS() +// TODO: https://chromedevtools.github.io/devtools-protocol/tot/Network/#method-setBlockedURLs (experimental for now). func listenForEventRequestPaused(ctx context.Context, logger *zap.Logger, allowList *regexp2.Regexp, denyList *regexp2.Regexp) { chromedp.ListenTarget(ctx, func(ev interface{}) { switch e := ev.(type) { @@ -95,24 +96,48 @@ func listenForEventResponseReceived(ctx context.Context, logger *zap.Logger, url }) } -// listenForEventLoadingFailedOnConnectionRefused listens for an event -// indicating that the main page failed to load. -// See https://github.com/gotenberg/gotenberg/issues/913. -func listenForEventLoadingFailedOnConnectionRefused(ctx context.Context, logger *zap.Logger, connectionRefused *error, connectionRefusedMu *sync.RWMutex) { +// listenForEventLoadingFailed listens for an event indicating that the main +// page failed to load. +// See: +// https://github.com/gotenberg/gotenberg/issues/913. +// https://github.com/gotenberg/gotenberg/issues/959. +func listenForEventLoadingFailed(ctx context.Context, logger *zap.Logger, loadingFailed *error, loadingFailedMu *sync.RWMutex) { chromedp.ListenTarget(ctx, func(ev interface{}) { switch ev := ev.(type) { case *network.EventLoadingFailed: logger.Debug(fmt.Sprintf("event EventLoadingFailed fired: %+v", ev.ErrorText)) - if ev.ErrorText != "net::ERR_CONNECTION_REFUSED" || ev.Type != network.ResourceTypeDocument { - logger.Debug("skip EventLoadingFailed: is not net::ERR_CONNECTION_REFUSED and/or resource type Document") + if ev.Type != network.ResourceTypeDocument { + logger.Debug("skip EventLoadingFailed: is not resource type Document") return } - connectionRefusedMu.Lock() - defer connectionRefusedMu.Unlock() + // Supposition: except iframe, an event loading failed with a + // resource type Document is about the main page. - *connectionRefused = fmt.Errorf("%s", ev.ErrorText) + // We are looking for common errors. + // TODO: sufficient? + errors := []string{ + "net::ERR_CONNECTION_CLOSED", + "net::ERR_CONNECTION_RESET", + "net::ERR_CONNECTION_REFUSED", + "net::ERR_CONNECTION_ABORTED", + "net::ERR_CONNECTION_FAILED", + "net::ERR_NAME_NOT_RESOLVED", + "net::ERR_INTERNET_DISCONNECTED", + "net::ERR_ADDRESS_UNREACHABLE", + "net::ERR_BLOCKED_BY_CLIENT", + "net::ERR_BLOCKED_BY_RESPONSE", + } + if !slices.Contains(errors, ev.ErrorText) { + logger.Debug(fmt.Sprintf("skip EventLoadingFailed: '%s' is not part of %+v", ev.ErrorText, errors)) + return + } + + loadingFailedMu.Lock() + defer loadingFailedMu.Unlock() + + *loadingFailed = fmt.Errorf("%s", ev.ErrorText) } }) } diff --git a/pkg/modules/chromium/routes.go b/pkg/modules/chromium/routes.go index 6edee141..ba1e7be2 100644 --- a/pkg/modules/chromium/routes.go +++ b/pkg/modules/chromium/routes.go @@ -688,12 +688,12 @@ func handleChromiumError(err error, options Options) error { ) } - if errors.Is(err, ErrConnectionRefused) { + if errors.Is(err, ErrLoadingFailed) { return api.WrapError( err, api.NewSentinelHttpError( http.StatusBadRequest, - "Chromium returned net::ERR_CONNECTION_REFUSED", + fmt.Sprintf("Chromium returned %v", err), ), ) } diff --git a/pkg/modules/chromium/routes_test.go b/pkg/modules/chromium/routes_test.go index e09c161e..ba957e89 100644 --- a/pkg/modules/chromium/routes_test.go +++ b/pkg/modules/chromium/routes_test.go @@ -1436,10 +1436,10 @@ func TestConvertUrl(t *testing.T) { expectOutputPathsCount: 0, }, { - scenario: "ErrConnectionRefused", + scenario: "ErrLoadingFailed", ctx: &api.ContextMock{Context: new(api.Context)}, api: &ApiMock{PdfMock: func(ctx context.Context, logger *zap.Logger, url, outputPath string, options PdfOptions) error { - return ErrConnectionRefused + return ErrLoadingFailed }}, options: DefaultPdfOptions(), expectError: true, @@ -1646,10 +1646,10 @@ func TestScreenshotUrl(t *testing.T) { expectOutputPathsCount: 0, }, { - scenario: "ErrConnectionRefused", + scenario: "ErrLoadingFailed", ctx: &api.ContextMock{Context: new(api.Context)}, api: &ApiMock{ScreenshotMock: func(ctx context.Context, logger *zap.Logger, url, outputPath string, options ScreenshotOptions) error { - return ErrConnectionRefused + return ErrLoadingFailed }}, options: DefaultScreenshotOptions(), expectError: true,