Bug 32335 – [v8] in Debug builds create an additional handle scope when doing debug build checks

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

[anton muhin]

As of now, v8::Object::GetPointerFromInternalField doesn't require handle scope.  However, in Debug builds we do some checks before accessing it which require handle scope (as they use GetInternalField method, for example).  Thus we need an additional handle scope in debug builds (only).

[anton muhin]

Created attachment 44551 [details]
first take

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 44551 [details] without any errors.

[Adam Barth]

Comment on attachment 44551 [details]
first take

Seems like the ASSERTS can be outside of the #ifdef.  How much does it cost to create the handle scope unconditionally?  I'd rather not have NDEBUG everywhere....   Can we create the handle scope at a lower level where we're already splitting on NDEBUG?

[anton muhin]

(In reply to comment #3)
> (From update of attachment 44551 [details])
> Seems like the ASSERTS can be outside of the #ifdef.

Technically yes.  However, if I remember it right, one can compile WebKit w/ asserts, but in Release mode, so we'll be in the trouble anyway.

Having said that, I'd easily adjust.

> How much does it cost to create the handle scope unconditionally?  I'd rather not have NDEBUG everywhere....

That's a hot path, and handle scope is only needed due to ASSERTS, so I'd rather not spend a CPU cycle on it.  If you don't mind.

> Can we create the handle scope at a lower level where we're
> already splitting on NDEBUG?

Yes, for maybeDOMWrapper, but there is this type check, alas.

Overall, I wholeheartedly agree those NDEBUG are ugly.  So just let me know how you'd like that to be arranged.

[Adam Barth]

Comment on attachment 44551 [details]
first take

Ah!  I misunderstood.  It's the ASSERTS themselves that require the handle scope.  Yeah, I don't see a more aesthetic way to do this.  Thanks for explaining it to me.

[anton muhin]

(In reply to comment #5)
> (From update of attachment 44551 [details])
> Ah!  I misunderstood.  It's the ASSERTS themselves that require the handle
> scope.  Yeah, I don't see a more aesthetic way to do this.  Thanks for
> explaining it to me.

Thanks a lot for review, Adam, cq+'ing.

[WebKit Commit Bot]

Comment on attachment 44551 [details]
first take

Rejecting patch 44551 from commit-queue.

Found no modified ChangeLogs, cannot create a commit message.
All changes require a ChangeLog.  See:
http://webkit.org/coding/contributing.html

[Eric Seidel (no email)]

Comment on attachment 44551 [details]
first take

Lets see if the failure reproduces.  If it does, then I'll file a bug and look at the machine itself.

[WebKit Commit Bot]

Comment on attachment 44551 [details]
first take

Rejecting patch 44551 from commit-queue.

Found no modified ChangeLogs, cannot create a commit message.
All changes require a ChangeLog.  See:
http://webkit.org/coding/contributing.html

[Eric Seidel (no email)]

I'm surprised the failure didn't occur earlier.  It looks like svn-apply fails with this patch:

bugzilla-tool apply-attachment 44551 [details]

I wonder if however you generated this git diff is different than how our scripts expect?

[anton muhin]

(In reply to comment #10)
> I'm surprised the failure didn't occur earlier.  It looks like svn-apply fails
> with this patch:
> 
> bugzilla-tool apply-attachment 44551 [details]
> 
> I wonder if however you generated this git diff is different than how our
> scripts expect?

git diff --no-prefix

Uploading a new patch (generated with plain git diff).

Just in case:

zsh/3 127 --% git --version
git version 1.5.4.3

[anton muhin]

Created attachment 44683 [details]
plain git diff

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 44683 [details] without any errors.

[WebKit Commit Bot]

Comment on attachment 44683 [details]
plain git diff

Clearing flags on attachment: 44683

Committed r51995: <http://trac.webkit.org/changeset/51995>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[anton muhin]

So yes, git diff --no-prefix was to blame.

[Eric Seidel (no email)]

I've filed bug 32438 about the svn-apply bug.

[Eric Seidel (no email)]

Btw, "bugzilla-tool post-diff" or "bugzilla-tool post-commits" will do the "right thing" regarding git patches.  See "bugzilla-tool help" for more information.  You might find one of those commands useful to you when posting patches in the future.

[anton muhin]

(In reply to comment #18)
> Btw, "bugzilla-tool post-diff" or "bugzilla-tool post-commits" will do the
> "right thing" regarding git patches.  See "bugzilla-tool help" for more
> information.  You might find one of those commands useful to you when posting
> patches in the future.

Thank you very much for pointing it out---never heard of it.