Bug 161726 – ScriptRunner should be driven by PendingScript rather than ScriptElement

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore JavaScript

[Yusuke Suzuki]

ScriptElement::notifyFinished is only used when the script tag has defer / async.
It is tricky. We should use PendingScript's setClient,clearClient and drive ScriptRunner by itself (as the same to HTMLScriptRunner).

[Yusuke Suzuki]

Created attachment 288241 [details]
Patch

[Yusuke Suzuki]

Created attachment 288242 [details]
Patch

[Yusuke Suzuki]

Created attachment 288244 [details]
Patch

[Yusuke Suzuki]

ah, i'll apply the slight change.
before calling clearClient, check whether it is watches.

[Ryosuke Niwa]

(In reply to comment #4)
> ah, i'll apply the slight change.
> before calling clearClient, check whether it is watches.

Do you want to upload a new patch before I review it?

[Yusuke Suzuki]

Created attachment 288251 [details]
Patch

[Yusuke Suzuki]

(In reply to comment #5)
> (In reply to comment #4)
> > ah, i'll apply the slight change.
> > before calling clearClient, check whether it is watches.
> 
> Do you want to upload a new patch before I review it?

I've just uploaded the patch :D! Now, ready for your review, thanks!

[Ryosuke Niwa]

Comment on attachment 288251 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=288251&action=review

r=me assuming you revert the change for IgnoreDestructiveWriteCountIncrementer in executeScriptForScriptRunner or explain why that's correct.

> Source/WebCore/dom/ScriptElement.cpp:-370
> -    IgnoreDestructiveWriteCountIncrementer ignoreDestructiveWriteCountIncrementer(&m_element.document());

I don't think this is correct because we're dispatching load event in this function.
We need the counter to be non-zero while dispatching that event.

[Yusuke Suzuki]

Comment on attachment 288251 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=288251&action=review

Thanks! I'll add the test to ensure the change.

>> Source/WebCore/dom/ScriptElement.cpp:-370
>> -    IgnoreDestructiveWriteCountIncrementer ignoreDestructiveWriteCountIncrementer(&m_element.document());
> 
> I don't think this is correct because we're dispatching load event in this function.
> We need the counter to be non-zero while dispatching that event.

Nice catch!!!
According to the spec[1], when firing the load event / the error event, this count should be already decremented.
So, the old behavior was incorrect. We should have the test to ensure that, thanks!

[1]: https://html.spec.whatwg.org/multipage/scripting.html#script-processing-model

[Yusuke Suzuki]

Comment on attachment 288251 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=288251&action=review

>>> Source/WebCore/dom/ScriptElement.cpp:-370
>>> -    IgnoreDestructiveWriteCountIncrementer ignoreDestructiveWriteCountIncrementer(&m_element.document());
>> 
>> I don't think this is correct because we're dispatching load event in this function.
>> We need the counter to be non-zero while dispatching that event.
> 
> Nice catch!!!
> According to the spec[1], when firing the load event / the error event, this count should be already decremented.
> So, the old behavior was incorrect. We should have the test to ensure that, thanks!
> 
> [1]: https://html.spec.whatwg.org/multipage/scripting.html#script-processing-model

Correct link is this. https://html.spec.whatwg.org/multipage/scripting.html#execute-the-script-block

[Yusuke Suzuki]

Committed r205652: <http://trac.webkit.org/changeset/205652>

[WebKit Commit Bot]

Re-opened since this is blocked by bug 161748

[Yusuke Suzuki]

ASan crash was due to the bug in HashTable, not related to this patch.
Now, the fix is landed in r205694. Let's re-land it as is.

[Yusuke Suzuki]

Committed r205695: <http://trac.webkit.org/changeset/205695>