Bug 40968 – Make PendingScript hold a CachedResourceClient open for its lifetime

• Status: RESOLVED
• Resolution: FIXED
• Component: Page Loading

[Tony Gentilcore]

Make PendingScript hold a CachedResourceClient open for its lifetime

[Tony Gentilcore]

Created attachment 59340 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 59340 [details]
Patch

WebCore/html/HTML5ScriptRunner.h:31
 +  #include "CachedScript.h"
Would be better to do this in the .cpp.

[Eric Seidel (no email)]

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.

[Tony Gentilcore]

Created attachment 59370 [details]
Patch

[Eric Seidel (no email)]

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

[Tony Gentilcore]

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

[Adam Barth]

Committed r61638: <http://trac.webkit.org/changeset/61638>

[WebKit Review Bot]

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

[Eric Seidel (no email)]

I think this caused 2 test cases to crash.

[Tony Gentilcore]

Created attachment 59455 [details]
Rollback

[Tony Gentilcore]

Rollback tested by building and running http/tests/security/frame-loading-via-document-write.html on Windows.

[Tony Gentilcore]

reopening for rollback

[Eric Seidel (no email)]

Comment on attachment 59455 [details]
Rollback

ChangeLogs are append-only.

[Eric Seidel (no email)]

Committed r61646: <http://trac.webkit.org/changeset/61646>

[Eric Seidel (no email)]

Rolled out in above commit.

[Adam Barth]

Sorry Tony.  I just renamed everything under the sun.  :(

[Mark Rowe (bdash)]

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!

[Adam Barth]

Thanks Mark.  That's very helpful.

[Tony Gentilcore]

Created attachment 59722 [details]
Patch

[WebKit Review Bot]

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.

[Eric Seidel (no email)]

Comment on attachment 59722 [details]
Patch

WebCore/ChangeLog:14
 +          No new tests because making a CachedResource purse itself is not
purge

LGTM.

[Tony Gentilcore]

Created attachment 59725 [details]
Patch for landing

[Alexey Proskuryakov]

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?

[Tony Gentilcore]

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

[Eric Seidel (no email)]

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

[Tony Gentilcore]

(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

[WebKit Commit Bot]

Comment on attachment 59725 [details]
Patch for landing

Clearing flags on attachment: 59725

Committed r61940: <http://trac.webkit.org/changeset/61940>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.