Bug 31180 - [v8] Do not retrieve handles in near death or empty state from the string impl cache
Summary: [v8] Do not retrieve handles in near death or empty state from the string imp...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: anton muhin
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-11-05 11:25 PST by anton muhin
Modified: 2010-01-18 11:29 PST (History)
2 users (show)

See Also:


Attachments
First take (1.33 KB, patch)
2009-11-05 11:29 PST, anton muhin
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description anton muhin 2009-11-05 11:25:30 PST
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).
Comment 1 anton muhin 2009-11-05 11:29:03 PST
Created attachment 42581 [details]
First take
Comment 2 Adam Barth 2009-11-05 11:45:19 PST
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?
Comment 3 anton muhin 2009-11-05 15:19:56 PST
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?
Comment 4 Adam Barth 2009-11-05 20:17:28 PST
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.
Comment 5 WebKit Commit Bot 2009-11-20 11:14:01 PST
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 ...
Comment 6 Eric Seidel (no email) 2009-11-20 11:32:18 PST
Comment on attachment 42581 [details]
First take

This rejection makes no sense.  I'll investigate.
Comment 7 WebKit Commit Bot 2009-11-20 11:49:34 PST
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
Comment 8 anton muhin 2009-11-20 12:15:14 PST
(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.
Comment 9 Eric Seidel (no email) 2009-12-28 22:40:09 PST
Attachment 42581 [details] was posted by a committer and has review+, assigning to Anton Muhin for commit.
Comment 10 Eric Seidel (no email) 2010-01-06 23:37:18 PST
Comment on attachment 42581 [details]
First take

I think that rejection was bogus.  Marking cq+ again.
Comment 11 WebKit Commit Bot 2010-01-07 00:22:11 PST
Comment on attachment 42581 [details]
First take

Clearing flags on attachment: 42581

Committed r52905: <http://trac.webkit.org/changeset/52905>
Comment 12 WebKit Commit Bot 2010-01-07 00:22:16 PST
All reviewed patches have been landed.  Closing bug.
Comment 13 anton muhin 2010-01-11 08:38:52 PST
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.
Comment 14 Eric Seidel (no email) 2010-01-11 12:06:05 PST
Please do not leave patches up for review, or even worse, r+'d that you do not want committed. :)
Comment 15 anton muhin 2010-01-18 09:59:03 PST
(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 :)
Comment 16 Eric Seidel (no email) 2010-01-18 11:29:27 PST
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!