Bug 31180 – [v8] Do not retrieve handles in near death or empty state from the string impl cache

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[anton muhin]

Due to nested GCs, there is a low chance of weak reference callback for external string being invoked while this string still referenced.  Most probably that is due to StringImpl cache (it allows to 'revive' an external string).

[anton muhin]

Created attachment 42581 [details]
First take

[Adam Barth]

Comment on attachment 42581 [details]
First take

+ v8::Persistent<v8::String> handle(cachedV8String);

Why a Persistent handle?  Can we use a local handle here?

Can we test this?

[anton muhin]

First of all, I've found out that my previous fix didn't make into 4.0.223.16, so I'd ask you to put this one on hold (I'd appreciate if you review+ it when found in good shape, but I don't want it to be committed, at least for now).

(In reply to comment #2)
> (From update of attachment 42581 [details])
> + v8::Persistent<v8::String> handle(cachedV8String);
> 
> Why a Persistent handle?  Can we use a local handle here?

::IsNearDeath is only present on Persistent handles (Locals are always alive).  If you're afraid of performance applications, AFAIK Persistent ctor (but not ::New) is roughly free.

> Can we test this?

I am lacking knowledge of our testing infrastructure.  The easiest way to check it would be to write some notable amount of C++ code (to create an object with a special weak reference callback, etc.) and expose it into JS.

Have we got something like that?

[Adam Barth]

Comment on attachment 42581 [details]
First take

No, we don't have any testing infrastructure like that.  I don't see how we could test this change.

[WebKit Commit Bot]

Comment on attachment 42581 [details]
First take

Rejecting patch 42581 from commit-queue.

Failed to run "['git', 'svn', 'dcommit']" exit_code: 1
Committing to http://svn.webkit.org/repository/webkit/trunk ...

[Eric Seidel (no email)]

Comment on attachment 42581 [details]
First take

This rejection makes no sense.  I'll investigate.

[WebKit Commit Bot]

Comment on attachment 42581 [details]
First take

Rejecting patch 42581 from commit-queue.

Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--quiet', '--exit-after-n-failures=1']" exit_code: 1
Running build-dumprendertree
Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests
Testing 11649 test cases.
inspector/console-format.html -> crashed

Exiting early after 1 failures. 9345 tests run.
490.34s total testing time

9344 test cases (99%) succeeded
1 test case (<1%) crashed
5 test cases (<1%) had stderr output

[anton muhin]

(In reply to comment #7)
> (From update of attachment 42581 [details])
> Rejecting patch 42581 from commit-queue.
> 
> Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari',
> '--quiet', '--exit-after-n-failures=1']" exit_code: 1
> Running build-dumprendertree
> Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests
> Testing 11649 test cases.
> inspector/console-format.html -> crashed
> 
> Exiting early after 1 failures. 9345 tests run.
> 490.34s total testing time
> 
> 9344 test cases (99%) succeeded
> 1 test case (<1%) crashed
> 5 test cases (<1%) had stderr output

Wow, thanks a lot, but that's something new.  Probably patch just rotted.  I'll double check if I can reproduce it locally.  Thanks again.

[Eric Seidel (no email)]

Attachment 42581 [details] was posted by a committer and has review+, assigning to Anton Muhin for commit.

[Eric Seidel (no email)]

Comment on attachment 42581 [details]
First take

I think that rejection was bogus.  Marking cq+ again.

[WebKit Commit Bot]

Comment on attachment 42581 [details]
First take

Clearing flags on attachment: 42581

Committed r52905: <http://trac.webkit.org/changeset/52905>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[anton muhin]

Oops.  I intentionally didn't commit it as I wasn't 100% sure it's necessary to fix the bug (and that bug never reappeared since then), but it slowed us down.  Let me double check and probably I would need to revert it back.

[Eric Seidel (no email)]

Please do not leave patches up for review, or even worse, r+'d that you do not want committed. :)

[anton muhin]

(In reply to comment #14)
> Please do not leave patches up for review, or even worse, r+'d that you do not
> want committed. :)

I see, sorry.  I wanted that to be submittable on instant, but as I can see now I abused the system.

The case is probably too rare to have any guidelines for it, but if there is something immediate you or any other remembers, I'd be glad to learn that :)

[Eric Seidel (no email)]

Oh, it's not a big deal at all.  And no, sadly we don't really have guidelines for this sort of thing.  We don't really have a "ready to commit, but commit blocked" state, but we should probably eventually create one just for this kind of patch (as it's rather common).  :)  Thanks again for the patch!