Bug 34530

Summary: [v8] Remove clear method from DOM object maps
Product: WebKit Reporter: anton muhin <antonm>
Component: WebCore Misc.Assignee: Nobody <webkit-unassigned>
Status: RESOLVED WONTFIX    
Severity: Normal CC: abarth, andersca, commit-queue, dglazkov, eric, senorblanco
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Other   
OS: All   
Attachments:
Description Flags
Patch none

Description anton muhin 2010-02-03 08:13:56 PST 
[v8] Remove clear method from DOM object maps
Comment 1 anton muhin 2010-02-03 08:17:11 PST 
Created attachment 48034 [details]
Patch
Comment 2 anton muhin 2010-02-03 08:21:14 PST 
Currently the only client of clear method is WorkerScriptController (this method is called in dtor.)

I believe that as worker most probably would own a separate V8 instance, VM destroy should take care of clearing (if it's not the case, I'd be glad to fix it on V8-side.)

If this cleanup is crucial, I'd provide a separate method to iterate through created wrappers.

This patch is just one in (planned) series of patches which could lead to elimination of indirect reference from the Node to its wrapper.  The next move would be to get rid of visit which is somewhat more involved.
Comment 3 Adam Barth 2010-02-03 18:33:40 PST 
Comment on attachment 48034 [details]
Patch

Amazing!  I take it this functionality had no clients.  :)
Comment 4 anton muhin 2010-02-04 04:40:23 PST 
Thanks a lot for review, Adam.

just in case, if I manage to get rid of indirection, ChunkedTable would go away as well---I remember about my promise :)
Comment 5 WebKit Commit Bot 2010-02-04 04:57:21 PST 
Comment on attachment 48034 [details]
Patch

Clearing flags on attachment: 48034

Committed r54341: <http://trac.webkit.org/changeset/54341>
Comment 6 WebKit Commit Bot 2010-02-04 04:57:30 PST 
All reviewed patches have been landed.  Closing bug.
Comment 7 Stephen White 2010-02-04 12:08:49 PST 
Sorry, I had to revert this, since it broke the ui_tests fpr Workers.  See 

http://build.chromium.org/buildbot/waterfall/builders/XP%20Tests%20(dbg)(2)/builds/13955/steps/ui_tests/logs/stdio

---

[ RUN      ] WorkerTest.MultipleWorkers
C:\b\slave\chromium-dbg-builder\build\src\chrome\test\ui\ui_test.cc(167): error: Value of: actual_crashes
  Actual: 1
Expected: expected_crashes_
Which is: 0
Encountered an unexpected crash in the program during this test.
[  FAILED  ] WorkerTest.MultipleWorkers (3609 ms)
[ RUN      ] WorkerTest.IncognitoSharedWorkers
C:\b\slave\chromium-dbg-builder\build\src\chrome\test\ui\ui_test.cc(167): error: Value of: actual_crashes
  Actual: 1
Expected: expected_crashes_
Which is: 0
Encountered an unexpected crash in the program during this test.
[  FAILED  ] WorkerTest.IncognitoSharedWorkers (3985 ms)
[ RUN      ] WorkerTest.WorkerFastLayoutTests2
Test: worker-call.html
Test: worker-constructor.html
Test: worker-event-listener.html
Test: worker-messageport.html
Test: worker-replace-global-constructor.html
Test: worker-terminate.html
C:\b\slave\chromium-dbg-builder\build\src\chrome\test\ui\ui_test.cc(167): error: Value of: actual_crashes
  Actual: 6
Expected: expected_crashes_
Which is: 0
Encountered an unexpected crash in the program during this test.
[  FAILED  ] WorkerTest.WorkerFastLayoutTests2 (10297 ms)
Comment 8 anton muhin 2010-02-04 12:18:31 PST 
Sorry about that---I ran ui_tests on my box and they were fine.  Will have a closer look.

(In reply to comment #7)
> Sorry, I had to revert this, since it broke the ui_tests fpr Workers.  See 
> 
> http://build.chromium.org/buildbot/waterfall/builders/XP%20Tests%20(dbg)(2)/builds/13955/steps/ui_tests/logs/stdio
> 
> ---
> 
> [ RUN      ] WorkerTest.MultipleWorkers
> C:\b\slave\chromium-dbg-builder\build\src\chrome\test\ui\ui_test.cc(167):
> error: Value of: actual_crashes
>   Actual: 1
> Expected: expected_crashes_
> Which is: 0
> Encountered an unexpected crash in the program during this test.
> [  FAILED  ] WorkerTest.MultipleWorkers (3609 ms)
> [ RUN      ] WorkerTest.IncognitoSharedWorkers
> C:\b\slave\chromium-dbg-builder\build\src\chrome\test\ui\ui_test.cc(167):
> error: Value of: actual_crashes
>   Actual: 1
> Expected: expected_crashes_
> Which is: 0
> Encountered an unexpected crash in the program during this test.
> [  FAILED  ] WorkerTest.IncognitoSharedWorkers (3985 ms)
> [ RUN      ] WorkerTest.WorkerFastLayoutTests2
> Test: worker-call.html
> Test: worker-constructor.html
> Test: worker-event-listener.html
> Test: worker-messageport.html
> Test: worker-replace-global-constructor.html
> Test: worker-terminate.html
> C:\b\slave\chromium-dbg-builder\build\src\chrome\test\ui\ui_test.cc(167):
> error: Value of: actual_crashes
>   Actual: 6
> Expected: expected_crashes_
> Which is: 0
> Encountered an unexpected crash in the program during this test.
> [  FAILED  ] WorkerTest.WorkerFastLayoutTests2 (10297 ms)
Comment 9 anton muhin 2010-02-08 03:43:12 PST 
That's strange.  I ran ui_tests couple of times on both Ubuntu and Windows 7 and didn't see anything like that.  Could it have been some flakiness?  Do we have any trybots for ui_tests?

(In reply to comment #8)
> Sorry about that---I ran ui_tests on my box and they were fine.  Will have a
> closer look.
> 
> (In reply to comment #7)
> > Sorry, I had to revert this, since it broke the ui_tests fpr Workers.  See 
> > 
> > http://build.chromium.org/buildbot/waterfall/builders/XP%20Tests%20(dbg)(2)/builds/13955/steps/ui_tests/logs/stdio
> > 
> > ---
> > 
> > [ RUN      ] WorkerTest.MultipleWorkers
> > C:\b\slave\chromium-dbg-builder\build\src\chrome\test\ui\ui_test.cc(167):
> > error: Value of: actual_crashes
> >   Actual: 1
> > Expected: expected_crashes_
> > Which is: 0
> > Encountered an unexpected crash in the program during this test.
> > [  FAILED  ] WorkerTest.MultipleWorkers (3609 ms)
> > [ RUN      ] WorkerTest.IncognitoSharedWorkers
> > C:\b\slave\chromium-dbg-builder\build\src\chrome\test\ui\ui_test.cc(167):
> > error: Value of: actual_crashes
> >   Actual: 1
> > Expected: expected_crashes_
> > Which is: 0
> > Encountered an unexpected crash in the program during this test.
> > [  FAILED  ] WorkerTest.IncognitoSharedWorkers (3985 ms)
> > [ RUN      ] WorkerTest.WorkerFastLayoutTests2
> > Test: worker-call.html
> > Test: worker-constructor.html
> > Test: worker-event-listener.html
> > Test: worker-messageport.html
> > Test: worker-replace-global-constructor.html
> > Test: worker-terminate.html
> > C:\b\slave\chromium-dbg-builder\build\src\chrome\test\ui\ui_test.cc(167):
> > error: Value of: actual_crashes
> >   Actual: 6
> > Expected: expected_crashes_
> > Which is: 0
> > Encountered an unexpected crash in the program during this test.
> > [  FAILED  ] WorkerTest.WorkerFastLayoutTests2 (10297 ms)
Comment 10 Stephen White 2010-02-08 14:57:36 PST 
(In reply to comment #9)
> That's strange.  I ran ui_tests couple of times on both Ubuntu and Windows 7
> and didn't see anything like that.  Could it have been some flakiness?

It's possible it was flake, although the problems all went away with the revert.  I don't know enough about these tests to say.

> Do we have any trybots for ui_tests?

The regular trybots (win/mac/linux) run ui_tests, I believe.

It's possible that i
> 
> (In reply to comment #8)
> > Sorry about that---I ran ui_tests on my box and they were fine.  Will have a
> > closer look.
> > 
> > (In reply to comment #7)
> > > Sorry, I had to revert this, since it broke the ui_tests fpr Workers.  See 
> > > 
> > > http://build.chromium.org/buildbot/waterfall/builders/XP%20Tests%20(dbg)(2)/builds/13955/steps/ui_tests/logs/stdio
> > > 
> > > ---
> > > 
> > > [ RUN      ] WorkerTest.MultipleWorkers
> > > C:\b\slave\chromium-dbg-builder\build\src\chrome\test\ui\ui_test.cc(167):
> > > error: Value of: actual_crashes
> > >   Actual: 1
> > > Expected: expected_crashes_
> > > Which is: 0
> > > Encountered an unexpected crash in the program during this test.
> > > [  FAILED  ] WorkerTest.MultipleWorkers (3609 ms)
> > > [ RUN      ] WorkerTest.IncognitoSharedWorkers
> > > C:\b\slave\chromium-dbg-builder\build\src\chrome\test\ui\ui_test.cc(167):
> > > error: Value of: actual_crashes
> > >   Actual: 1
> > > Expected: expected_crashes_
> > > Which is: 0
> > > Encountered an unexpected crash in the program during this test.
> > > [  FAILED  ] WorkerTest.IncognitoSharedWorkers (3985 ms)
> > > [ RUN      ] WorkerTest.WorkerFastLayoutTests2
> > > Test: worker-call.html
> > > Test: worker-constructor.html
> > > Test: worker-event-listener.html
> > > Test: worker-messageport.html
> > > Test: worker-replace-global-constructor.html
> > > Test: worker-terminate.html
> > > C:\b\slave\chromium-dbg-builder\build\src\chrome\test\ui\ui_test.cc(167):
> > > error: Value of: actual_crashes
> > >   Actual: 6
> > > Expected: expected_crashes_
> > > Which is: 0
> > > Encountered an unexpected crash in the program during this test.
> > > [  FAILED  ] WorkerTest.WorkerFastLayoutTests2 (10297 ms)
Comment 11 Anders Carlsson 2013-09-12 22:29:19 PDT 
V8 is gone.