Make PendingScript hold a CachedResourceClient open for its lifetime
Created attachment 59340 [details] Patch
Comment on attachment 59340 [details] Patch WebCore/html/HTML5ScriptRunner.h:31 + #include "CachedScript.h" Would be better to do this in the .cpp.
Comment on attachment 59340 [details] Patch WebCore/html/HTML5ScriptRunner.cpp:57 + if (m_parsingBlockingScript.cachedScript() && m_parsingBlockingScript.watchingForLoad) I don't thin we should bother moving watchingForLoad() back to being a direct-access member. I suspect that PendingScript is only going to be moving more and more towards class-ness. I'm also not sure it maters. :) Move the implementation stuff into the cpp, and then I think this is good to go.
Created attachment 59370 [details] Patch
Comment on attachment 59370 [details] Patch LGTM! Note, no need for argument names when they don't add anything ( 78 void setCachedScript(CachedScript* cachedScript); ) No need to change it here though. We really need you to have commit-bit so nits are easier to fix on landing. :)
(In reply to comment #5) > (From update of attachment 59370 [details]) > LGTM! Note, no need for argument names when they don't add anything ( 78 void setCachedScript(CachedScript* cachedScript); > ) I think this is the second time I've done that. So I'll put together a style-nit patch to fix those problems in WebCore/html/HTML5* > > No need to change it here though. We really need you to have commit-bit so nits are easier to fix on landing. :) Paperwork is in the mail :)
Committed r61638: <http://trac.webkit.org/changeset/61638>
http://trac.webkit.org/changeset/61638 might have broken Qt Linux Release The following changes are on the blame list: http://trac.webkit.org/changeset/61640 http://trac.webkit.org/changeset/61638 http://trac.webkit.org/changeset/61639
I think this caused 2 test cases to crash.
Created attachment 59455 [details] Rollback
Rollback tested by building and running http/tests/security/frame-loading-via-document-write.html on Windows.
reopening for rollback
Comment on attachment 59455 [details] Rollback ChangeLogs are append-only.
Committed r61646: <http://trac.webkit.org/changeset/61646>
Rolled out in above commit.
Sorry Tony. I just renamed everything under the sun. :(
We happened to pick up r61638 in one of our internal builds before we noticed that it was rolled out. It was causing crashes in Safari too. I spent some time debugging and tracked it down to PendingScript’s lack of a copy constructor and assignment operator. The fact that HTMLScriptRunner::executePendingScript clears m_parsingBlockingScript by doing: m_parsingBlockingScript = PendingScript(); combined with the fact that PendingScript::setCachedScript unregisters / registers the PendingScript as a client of the CachedScript results in PendingScript instances sticking around as clients of CachedScripts even after they’re deleted, since they never have an opportunity to unregister themselves as a client. A trivial fix for that would be to add a clear method to PendingScript that uses setCachedScript(0) to clear the m_cachedScript member and just zeroes the other members, and then to adopt .clear() in place of the assignment operator above. Updating the class to inherit from Noncopyable will also avoid this mistake from being repeated. Hope that helps!
Thanks Mark. That's very helpful.
Created attachment 59722 [details] Patch
Attachment 59722 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1 WebCore/ChangeLog:13: Line contains tab character. [whitespace/tab] [5] Total errors found: 1 in 5 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 59722 [details] Patch WebCore/ChangeLog:14 + No new tests because making a CachedResource purse itself is not purge LGTM.
Created attachment 59725 [details] Patch for landing
Is the crash also untestable, or only the original fix isn't? We really need better testing hooks for WebCore resource cache. It's such a large and important area of code, and almost untested! Can a layoutTestController method be added that would make this testable?
(In reply to comment #23) > Is the crash also untestable, or only the original fix isn't? The crash showed up on two layout tests, so it is tested. This was landed without the CQ, the crashes showed up, and it was rolled back just a few revisions later. It was kind of a strange fluke that the nightly was cut in between the landing and the rollback. > > We really need better testing hooks for WebCore resource cache. It's such a large and important area of code, and almost untested! Totally agreed. It is scary. > > Can a layoutTestController method be added that would make this testable? Good idea. I think a method that purges all purgeable buffers would be perfect.
(In reply to comment #24) > > Can a layoutTestController method be added that would make this testable? > > Good idea. I think a method that purges all purgeable buffers would be perfect. Woh, that would be useful! We should at least file a bug for that.
(In reply to comment #25) > (In reply to comment #24) > > > Can a layoutTestController method be added that would make this testable? > > > > Good idea. I think a method that purges all purgeable buffers would be perfect. > > Woh, that would be useful! We should at least file a bug for that. https://bugs.webkit.org/show_bug.cgi?id=41219
Comment on attachment 59725 [details] Patch for landing Clearing flags on attachment: 59725 Committed r61940: <http://trac.webkit.org/changeset/61940>
All reviewed patches have been landed. Closing bug.