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).
Created attachment 288241 [details] Patch
Created attachment 288242 [details] Patch
Created attachment 288244 [details] Patch
ah, i'll apply the slight change. before calling clearClient, check whether it is watches.
(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?
Created attachment 288251 [details] Patch
(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!
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.
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
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
Committed r205652: <http://trac.webkit.org/changeset/205652>
Re-opened since this is blocked by bug 161748
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.
Committed r205695: <http://trac.webkit.org/changeset/205695>