Bug 163775 – Move some more assorted classes from ExceptionCode to Exception

• Component: Bindings
• Status: RESOLVED FIXED

[Darin Adler]

Move some more assorted classes from ExceptionCode to Exception

[Darin Adler]

Created attachment 292324 [details]
Patch

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Darin Adler]

Created attachment 292356 [details]
Patch

[Darin Adler]

EWS tests are all passing now, so ready for review. The Mac EWS failure looks like a flaky crash, unrelated to this patch.

[Chris Dumez]

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*.

[Chris Dumez]

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.

[Darin Adler]

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.

[Chris Dumez]

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.

[Chris Dumez]

(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).

[Darin Adler]

Committed r207687: <http://trac.webkit.org/changeset/207687>

[Darin Adler]

(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.