Bug 194319

Summary:

Web Inspector: "Worker not found" uncaught protocol errors

Product:

WebKit

Reporter:

Joseph Pecoraro <joepeck>

Component:

Web Inspector

Assignee:

Joseph Pecoraro <joepeck>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

commit-queue, hi, inspector-bugzilla-changes, joepeck, mattbaker, webkit-bug-importer

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

All

OS:

All

Attachments:

Description	Flags
[PATCH] Proposed Fix	mattbaker: review+
[PATCH] For Landing	none

Joseph Pecoraro

Reported 2019-02-05 16:43:30 PST

"Worker not found" uncaught protocol errors Steps to Reproduce: 1. Inspect maps.google.com 2. Reload a few times => Errors, Worker was created+terminated in the backend before frontend could send messages, okay to not find the worker. Steps to Reproduce: 1. Inspect maps.google.com 2. Cross origin navigate to a page without workers => Potential Errors, leftover worker targets in WI.targets

Attachments
[PATCH] Proposed Fix (3.39 KB, patch) 2019-02-05 16:47 PST, Joseph Pecoraro	mattbaker: review+	Details Formatted Diff Diff
[PATCH] For Landing (3.47 KB, patch) 2019-02-06 11:19 PST, Joseph Pecoraro	no flags	Details Formatted Diff Diff
View All Add attachment proposed patch, testcase, etc.

Joseph Pecoraro

Comment 1 2019-02-05 16:47:04 PST

Created attachment 361248 [details] [PATCH] Proposed Fix

Matt Baker

Comment 2 2019-02-05 16:52:44 PST

Comment on attachment 361248 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=361248&action=review r=me > Source/WebInspectorUI/ChangeLog:12 > + frame, but for now this is the only a page transition can kill Grammar: "the only a page" -> "the only way a page" > Source/WebInspectorUI/UserInterface/Base/Main.js:232 > + let workerTargets = WI.targets.filter((x) => x.type === WI.Target.Type.Worker); Nit: rename x to target.

Joseph Pecoraro

Comment 3 2019-02-05 17:00:14 PST

(In reply to Matt Baker from comment #2) > Comment on attachment 361248 [details] > [PATCH] Proposed Fix > > View in context: > https://bugs.webkit.org/attachment.cgi?id=361248&action=review > > r=me > > > Source/WebInspectorUI/ChangeLog:12 > > + frame, but for now this is the only a page transition can kill > > Grammar: "the only a page" -> "the only way a page" Doh! Thanks > > Source/WebInspectorUI/UserInterface/Base/Main.js:232 > > + let workerTargets = WI.targets.filter((x) => x.type === WI.Target.Type.Worker); > > Nit: rename x to target. There is already a target I don't want to shadow, and I think it is clear from context.

Devin Rousso

Comment 4 2019-02-05 17:01:26 PST

Comment on attachment 361248 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=361248&action=review >> Source/WebInspectorUI/ChangeLog:12 >> + frame, but for now this is the only a page transition can kill > > Grammar: "the only a page" -> "the only way a page" err, I think he means "but for now only a page transition can kill all worker targets." >> Source/WebInspectorUI/UserInterface/Base/Main.js:232 >> + let workerTargets = WI.targets.filter((x) => x.type === WI.Target.Type.Worker); > > Nit: rename x to target. We don't want to use `target` since that already exists in this function. I think `item` is a better/clearer name. > Source/WebInspectorUI/UserInterface/Controllers/WorkerManager.js:58 > + WorkerAgent.initialized(workerId).catch(function(){}); Rather than just silently handling failure, maybe we can do an engineering-build-only log? Like a `console.assert(false, ...);`? > Source/WebInspectorUI/UserInterface/Protocol/Connection.js:311 > + // Ignore errors if a worker went away quickly. We should also have this comment in WorkerManager.js. > Source/WebInspectorUI/UserInterface/Protocol/Connection.js:312 > + WorkerAgent.sendMessageToWorker(this._workerId, message).catch(function(){}); Ditto (>WorkerManager.js:58).

Joseph Pecoraro

Comment 5 2019-02-06 11:18:41 PST

> > Source/WebInspectorUI/UserInterface/Controllers/WorkerManager.js:58 > > + WorkerAgent.initialized(workerId).catch(function(){}); > > Rather than just silently handling failure, maybe we can do an > engineering-build-only log? Like a `console.assert(false, ...);`? I don't want to assert(false) for something we know can and will happen. Jus loading maps.google.com for example.

Joseph Pecoraro

Comment 6 2019-02-06 11:19:42 PST

Created attachment 361308 [details] [PATCH] For Landing

WebKit Commit Bot

Comment 7 2019-02-06 11:57:44 PST

Comment on attachment 361308 [details] [PATCH] For Landing Clearing flags on attachment: 361308 Committed r241039: <https://trac.webkit.org/changeset/241039>

Radar WebKit Bug Importer

Comment 8 2019-02-06 13:02:53 PST

<rdar://problem/47860997>

Note You need to log in before you can comment on or make changes to this bug.