Move some more assorted classes from ExceptionCode to Exception
Created attachment 292324 [details] Patch
Comment on attachment 292324 [details] Patch Attachment 292324 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/2336314 New failing tests: crypto/workers/subtle/gc-worker.html crypto/workers/crypto-gc-worker.html
Created attachment 292325 [details] Archive of layout-test-results from ews101 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews101 Port: mac-yosemite Platform: Mac OS X 10.10.5
Comment on attachment 292324 [details] Patch Attachment 292324 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/2336327 New failing tests: crypto/workers/subtle/gc-worker.html crypto/workers/crypto-gc-worker.html
Created attachment 292326 [details] Archive of layout-test-results from ews107 for mac-yosemite-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews107 Port: mac-yosemite-wk2 Platform: Mac OS X 10.10.5
Comment on attachment 292324 [details] Patch Attachment 292324 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/2336320 New failing tests: crypto/workers/subtle/gc-worker.html crypto/workers/crypto-gc-worker.html
Created attachment 292328 [details] Archive of layout-test-results from ews113 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews113 Port: mac-yosemite Platform: Mac OS X 10.10.5
Comment on attachment 292324 [details] Patch Attachment 292324 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/2336333 New failing tests: crypto/workers/subtle/gc-worker.html crypto/workers/crypto-gc-worker.html
Created attachment 292329 [details] Archive of layout-test-results from ews125 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews125 Port: ios-simulator-wk2 Platform: Mac OS X 10.11.6
Created attachment 292356 [details] Patch
EWS tests are all passing now, so ready for review. The Mac EWS failure looks like a flaky crash, unrelated to this patch.
Comment on attachment 292356 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=292356&action=review > Source/WebCore/bindings/js/JSWorkerGlobalScopeCustom.cpp:59 > + visitor.addOpaqueRoot(&wrapped()); I think this change is bad. We get different pointer values now. We likely want to cast to ScriptExecutionContext*.
Comment on attachment 292356 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=292356&action=review r=me with comments. We especially need to add the static_cast<ScriptExecutionContext*>(&wapped()) before landing. > Source/WebCore/dom/MessagePort.h:87 > + bool isEntangled() { return !m_closed && !isNeutered(); } Should be const > Source/WebCore/dom/MessagePort.h:90 > + bool isNeutered() { return !m_entangledChannel; } should be const. > Source/WebCore/workers/WorkerGlobalScope.cpp:310 > + // FIXME: What do we accomplish by posting a task that does nothing? This is is used for synchronization with the worker thread below see the loop below: while (!done && waitResult != MessageQueueTerminated) > Source/WebCore/workers/WorkerGlobalScope.cpp:329 > + // FIXME: What do we accomplish by posting a task that does nothing? Same as above.
Comment on attachment 292356 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=292356&action=review >> Source/WebCore/bindings/js/JSWorkerGlobalScopeCustom.cpp:59 >> + visitor.addOpaqueRoot(&wrapped()); > > I think this change is bad. We get different pointer values now. We likely want to cast to ScriptExecutionContext*. I am almost certain this works. When I removed this line of code, the Crypto garbage collection test failed. When I added it back, the Crypto garbage collection test passed. So I am pretty sure it's doing its job. On the other hand, I am really unclear on why this is needed at all, so maybe I shouldn’t mess with it. I am happy to simply revert this to what the old code was doing. I understand how the change seems wrong. >> Source/WebCore/workers/WorkerGlobalScope.cpp:310 >> + // FIXME: What do we accomplish by posting a task that does nothing? > > This is is used for synchronization with the worker thread below see the loop below: > while (!done && waitResult != MessageQueueTerminated) I still don’t understand; done is set in the loader task, and I don’t understand how waitResult is affected by the task posted on the worker global scope queue, but I will remove the FIXME since you at least seem to understand the code. >> Source/WebCore/workers/WorkerGlobalScope.cpp:329 >> + // FIXME: What do we accomplish by posting a task that does nothing? > > Same as above. OK.
Comment on attachment 292356 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=292356&action=review >>> Source/WebCore/bindings/js/JSWorkerGlobalScopeCustom.cpp:59 >>> + visitor.addOpaqueRoot(&wrapped()); >> >> I think this change is bad. We get different pointer values now. We likely want to cast to ScriptExecutionContext*. > > I am almost certain this works. When I removed this line of code, the Crypto garbage collection test failed. When I added it back, the Crypto garbage collection test passed. So I am pretty sure it's doing its job. > > On the other hand, I am really unclear on why this is needed at all, so maybe I shouldn’t mess with it. I am happy to simply revert this to what the old code was doing. I understand how the change seems wrong. This needs to match what the children do, in this case Crypto. Crypto uses GenerateIsReachable=ImplScriptExecutionContext, so we generate: ScriptExecutionContext* root = WTF::getPtr(js${interfaceName}->wrapped().scriptExecutionContext()); return visitor.containsOpaqueRoot(root); Note that root is a ScriptExecutionContext* and that containsOpaqueRoot() takes a void*. So when we register the opaque root, we should also register a ScriptExecutionContext*, not a WorkerGlobalScope* and the pointer values may differ. And in this case, I think they do differ because WorkerGlobalScope inherits RefCounted first.
(In reply to comment #15) > Comment on attachment 292356 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=292356&action=review > > >>> Source/WebCore/bindings/js/JSWorkerGlobalScopeCustom.cpp:59 > >>> + visitor.addOpaqueRoot(&wrapped()); > >> > >> I think this change is bad. We get different pointer values now. We likely want to cast to ScriptExecutionContext*. > > > > I am almost certain this works. When I removed this line of code, the Crypto garbage collection test failed. When I added it back, the Crypto garbage collection test passed. So I am pretty sure it's doing its job. > > > > On the other hand, I am really unclear on why this is needed at all, so maybe I shouldn’t mess with it. I am happy to simply revert this to what the old code was doing. I understand how the change seems wrong. > > This needs to match what the children do, in this case Crypto. > > Crypto uses GenerateIsReachable=ImplScriptExecutionContext, so we generate: > ScriptExecutionContext* root = > WTF::getPtr(js${interfaceName}->wrapped().scriptExecutionContext()); > return visitor.containsOpaqueRoot(root); > > Note that root is a ScriptExecutionContext* and that containsOpaqueRoot() > takes a void*. > > So when we register the opaque root, we should also register a > ScriptExecutionContext*, not a WorkerGlobalScope* and the pointer values may > differ. And in this case, I think they do differ because WorkerGlobalScope > inherits RefCounted first. If it works, I do not understand how. I have made this kind of changes in the past without paying attention and it has lead to hard to investigate failures in the tests (usually flakes).
Committed r207687: <http://trac.webkit.org/changeset/207687>
(In reply to comment #16) > If it works, I do not understand how. It did work. I also do not understand how. I changed it based on your comments so we do not need to figure that out.