Bug 163775

Summary: Move some more assorted classes from ExceptionCode to Exception
Product: WebKit Reporter: Darin Adler <darin>
Component: BindingsAssignee: Darin Adler <darin>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, andersca, buildbot, cdumez, commit-queue, dbates, esprehn+autocc, kangil.han, kondapallykalyan, mkwst, rniwa, sam, youennf
Priority: P2    
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
Attachments:
Description Flags
Patch
none
Archive of layout-test-results from ews101 for mac-yosemite
none
Archive of layout-test-results from ews107 for mac-yosemite-wk2
none
Archive of layout-test-results from ews113 for mac-yosemite
none
Archive of layout-test-results from ews125 for ios-simulator-wk2
none
Patch cdumez: review+, cdumez: commit-queue-

Description Darin Adler 2016-10-20 23:24:55 PDT
Move some more assorted classes from ExceptionCode to Exception
Comment 1 Darin Adler 2016-10-20 23:46:03 PDT
Created attachment 292324 [details]
Patch
Comment 2 Build Bot 2016-10-21 00:48:12 PDT
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
Comment 3 Build Bot 2016-10-21 00:48:15 PDT
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 4 Build Bot 2016-10-21 00:52:39 PDT
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
Comment 5 Build Bot 2016-10-21 00:52:44 PDT
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 6 Build Bot 2016-10-21 00:56:09 PDT
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
Comment 7 Build Bot 2016-10-21 00:56:13 PDT
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 8 Build Bot 2016-10-21 01:03:35 PDT
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
Comment 9 Build Bot 2016-10-21 01:03:39 PDT
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
Comment 10 Darin Adler 2016-10-21 09:43:40 PDT
Created attachment 292356 [details]
Patch
Comment 11 Darin Adler 2016-10-21 10:40:45 PDT
EWS tests are all passing now, so ready for review. The Mac EWS failure looks like a flaky crash, unrelated to this patch.
Comment 12 Chris Dumez 2016-10-21 10:47:32 PDT
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 13 Chris Dumez 2016-10-21 11:02:23 PDT
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 14 Darin Adler 2016-10-21 11:22:45 PDT
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 15 Chris Dumez 2016-10-21 11:30:20 PDT
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.
Comment 16 Chris Dumez 2016-10-21 11:33:15 PDT
(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).
Comment 17 Darin Adler 2016-10-21 11:34:55 PDT
Committed r207687: <http://trac.webkit.org/changeset/207687>
Comment 18 Darin Adler 2016-10-21 22:39:47 PDT
(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.