|
|
Created:
6 years, 5 months ago by kouhei (in TOK) Modified:
6 years, 4 months ago Reviewers:
haraken, dcarney, jochen (gone - plz use gerrit) CC:
v8-dev Base URL:
git://github.com/v8/v8.git@master Project:
v8 Visibility:
Public. |
DescriptionBootstrapper::DetachGlobal also need to unset global_proxy's constructor to remove all refs to context
DetachGlobal detaches original context of a global proxy object.
Before this patch, the constructor JSFunction still carried a reference to the old context after |Bootstrapper::DetachGlobal| call.
This patch removes the reference by setting the constructor null.
TEST=http/tests/security/isolatedWorld w/ --enable-leak-detection
LOG=N
BUG=364377
R=dcarney@chromium.org
Committed: https://code.google.com/p/v8/source/detail?r=22460
Patch Set 1 #Patch Set 2 : modify copied map #
Created: 6 years, 5 months ago
Messages
Total messages: 16 (0 generated)
This is my other attempt to fix the isolated world bug. I inspected |V8WindowShell::m_global| after navigation, and found that it was still carrying a reference to the old context via |__proto__.constructor|. I think this fix is OK as the global_proxy's prototype is invisible from user, and the prototype is recovered in next call to |Genesis::CreatenewGlobals| anyway. However, I'm not an v8 expert so I'd appreciate if you would take a close look. Thanks!
dcarney: ping?(≧∇≦) We need to fix the document leak of isolated worlds since it happens on all Blink-in-JS.
On 2014/07/18 05:43:37, haraken wrote: > dcarney: ping?(≧∇≦) We need to fix the document leak of isolated worlds since it > happens on all Blink-in-JS. i think this is okay. i'll land it and see what happens. kouhei, did you run all the blink tests with this patch applied? lgtm
On 2014/07/18 05:59:03, dcarney wrote: > On 2014/07/18 05:43:37, haraken wrote: > > dcarney: ping?(≧∇≦) We need to fix the document leak of isolated worlds since > it > > happens on all Blink-in-JS. > > i think this is okay. i'll land it and see what happens. kouhei, did you run > all the blink tests with this patch applied? > > lgtm Yes, I run the layout tests: http://build.chromium.org/p/tryserver.v8/builders/v8_linux_layout_dbg/builds/284
Message was sent while issue was closed.
Committed patchset #2 manually as r22460 (presubmit successful).
Message was sent while issue was closed.
On 2014/07/18 06:06:25, dcarney wrote: > Committed patchset #2 manually as r22460 (presubmit successful). we're seeing some potential crashes from that cl on mac: http://build.chromium.org/p/client.v8/builders/Webkit%20Mac do you guys have time to investigate? i don't right now, and we'll revert the cl if we can't resolve this fairly soon
Message was sent while issue was closed.
On 2014/07/18 10:52:11, dcarney wrote: > On 2014/07/18 06:06:25, dcarney wrote: > > Committed patchset #2 manually as r22460 (presubmit successful). > > we're seeing some potential crashes from that cl on mac: > > http://build.chromium.org/p/client.v8/builders/Webkit%20Mac > > do you guys have time to investigate? i don't right now, and we'll revert the > cl if we can't resolve this fairly soon Japan is running into the weekend, so it would make sense to revert the CL. Sorry about that.
Message was sent while issue was closed.
On 2014/07/18 11:06:33, haraken wrote: > On 2014/07/18 10:52:11, dcarney wrote: > > On 2014/07/18 06:06:25, dcarney wrote: > > > Committed patchset #2 manually as r22460 (presubmit successful). > > > > we're seeing some potential crashes from that cl on mac: > > > > http://build.chromium.org/p/client.v8/builders/Webkit%20Mac > > > > do you guys have time to investigate? i don't right now, and we'll revert the > > cl if we can't resolve this fairly soon > > Japan is running into the weekend, so it would make sense to revert the CL. > Sorry about that. np - i'll revert and if it doesn't change the crashing i'll re-revert. i don't see how this cl could be at fault. have a good weekend
Message was sent while issue was closed.
> np - i'll revert and if it doesn't change the crashing i'll re-revert. i don't > see how this cl could be at fault. have a good weekend fyi - the revert fixed the crashing, so i'll leave it to you guys to fix on next week
I think this is resolved by blink r178818 haraken: Would you try landing this again?
On 2014/07/25 08:51:27, kouhei wrote: > I think this is resolved by blink r178818 > > haraken: Would you try landing this again? sure.
On 2014/07/25 08:51:50, haraken wrote: > On 2014/07/25 08:51:27, kouhei wrote: > > I think this is resolved by blink r178818 > > > > haraken: Would you try landing this again? > > sure. Should I commit to bleeding_edge or master? (Or I'd be happy if you could land this on behalf of us.)
I'm off for the next 2 months. jochen, could you land this and watch the tree for breakage?
i'm out today, but I'll take care of it on Monday (Kentaro, to answer your question: bleeding_edge. fetch v8 cd v8 git cl patch -b land 397953009 git cl dcommit -c '...' )
On 2014/07/25 10:10:53, jochen wrote: > i'm out today, but I'll take care of it on Monday (Kentaro, to answer your > question: bleeding_edge. > > fetch v8 > cd v8 > git cl patch -b land 397953009 > git cl dcommit -c '...' > > ) For safety, let me ask you to take care of landing the CL. Monday is totally fine :)
On 2014/07/25 10:12:49, haraken wrote: > On 2014/07/25 10:10:53, jochen wrote: > > i'm out today, but I'll take care of it on Monday (Kentaro, to answer your > > question: bleeding_edge. > > > > fetch v8 > > cd v8 > > git cl patch -b land 397953009 > > git cl dcommit -c '...' > > > > ) > > For safety, let me ask you to take care of landing the CL. Monday is totally > fine :) Thanks jochen! Relanded as https://codereview.chromium.org/424703002 |