1146316 - Remove nsWrapperCache::SetIsNotDOMBinding and IsDOMBinding()

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Boris Zbarsky [:bzbarsky]]

We need to move all the current consumers to webidl: sandbox and the various message manager bits.

Comment 1

[Peter Van der Beken [:peterv]]

Attachment: v1

Comment 2

[Peter Van der Beken [:peterv]]

Attachment: Preserve the wrapper of sandboxes, so that we never try to call WrapObject on them v1

Comment 3

[Peter Van der Beken [:peterv]]

Attachment: Remove nsWrapperCache::SetIsNotDOMBinding and IsDOMBinding() v2

Comment 4

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8967074 [details] [diff] [review]
Preserve the wrapper of sandboxes, so that we never try to call WrapObject on them v1

>+        JS_SetPrivate(global, sbp.forget().downcast<nsIScriptObjectPrincipal>().take());

Hmm.  So this class does:

     NS_DECL_CYCLE_COLLECTION_SCRIPT_HOLDER_CLASS_AMBIGUOUS(SandboxPrivate,
                                                            nsIGlobalObject)

and

  NS_INTERFACE_MAP_ENTRY_AMBIGUOUS(nsISupports, nsIScriptObjectPrincipal)

Shouldn't those two match?

In any case, whatever places need to match here (e.g. does the QI need to match the private value bit?) should really have comments pointing to each other.

It's also a bit weird to call downcast() when you're really doing an upcast; might deserve a comment there too.

r=me

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8967076 [details] [diff] [review]
Remove nsWrapperCache::SetIsNotDOMBinding and IsDOMBinding() v2

You should change WRAPPER_CACHE_FLAGS_BITS_USED to 1.  That adds a free bit on nodes, yay!

> +// Only set aAllowNativeWrapper to false if you really know you need it, if in

s/,/;/

>+          Throw(aCx, NS_ERROR_UNEXPECTED);
>+          return false;

Could just be:

  return Throw(aCx, NS_ERROR_UNEXPECTED);

> // Wrapping of our native parent, for cases when it's a WebIDL object (though
> // possibly preffed off).

There is no "preffed off" case now.

r=me.  Thank you for cleaning this up!

Comment 6

[Peter Van der Beken [:peterv]]

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #4)
> Hmm.  So this class does:
> 
>      NS_DECL_CYCLE_COLLECTION_SCRIPT_HOLDER_CLASS_AMBIGUOUS(SandboxPrivate,
>                                                             nsIGlobalObject)
> 
> and
> 
>   NS_INTERFACE_MAP_ENTRY_AMBIGUOUS(nsISupports, nsIScriptObjectPrincipal)
> 
> Shouldn't those two match?

No, the CC uses an nsISupports that may be different from the canonical nsISupports (for various reasons). I don't think it's worth commenting about that here, it's just a general rule (nsCycleCollectionParticipant.h has a comment about it).

> In any case, whatever places need to match here (e.g. does the QI need to
> match the private value bit?) should really have comments pointing to each
> other.

I don't think anything relies on the private being the canonical nsISupports. I added comments to Create and GetPrivate that the type needs to match.

> It's also a bit weird to call downcast() when you're really doing an upcast;
> might deserve a comment there too.

I switched to ToSupports instead.

Comment 7

[Peter Van der Beken [:peterv]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1316c78daeb6fd1883b3c1688c2591ee02c4d3e5
Bug 1146316 - Preserve the wrapper of sandboxes, so that we never try to call WrapObject on them. r=bz.

https://hg.mozilla.org/integration/mozilla-inbound/rev/719f7596c208eab1f2c8a1a250f1a6d2a7304d65
Bug 1146316 - Remove nsWrapperCache::SetIsNotDOMBinding and IsDOMBinding(). r=bz.

Comment 8

[Dorel Luca [:dluca]]

Backed out 8 changesets (bug 1453011, bug 1452981, bug 1146316) For xpcshell and mochitest failures on multiple files. CLOSED TREE

Log of the failure:
https://treeherder.mozilla.org/logviewer.html#?job_id=174810370&repo=mozilla-inbound&lineNumber=1577

09:36:27     INFO -  TEST-PASS | devtools/client/performance/test/unit/test_jit-graph-data.js | took 12963ms
09:36:27     INFO -  TEST-START | devtools/client/performance/test/unit/test_tree-model-06.js
09:36:31     INFO -  TEST-PASS | devtools/client/performance/test/unit/test_tree-model-02.js | took 11871ms
09:36:31     INFO -  TEST-START | devtools/client/performance/test/unit/test_tree-model-07.js
09:36:31     INFO -  mozcrash Saved minidump as /Users/cltbld/tasks/task_1524241879/build/blobber_upload_dir/F9603462-C1C9-4C82-BB9E-546B04DCD948.dmp
09:36:31     INFO -  mozcrash Saved app info as /Users/cltbld/tasks/task_1524241879/build/blobber_upload_dir/F9603462-C1C9-4C82-BB9E-546B04DCD948.extra
09:36:31  WARNING -  PROCESS-CRASH | xpcshell-remote.ini:browser/components/extensions/test/xpcshell/test_ext_geckoProfiler_control.js | application crashed [@ ProcessExecutableMemory::release()]
09:36:31     INFO -  Crash dump filename: /var/folders/x8/56fhxqzs7rx3spfwdg5l65t800000w/T/xpc-other-Wm7KED/F9603462-C1C9-4C82-BB9E-546B04DCD948.dmp
09:36:31     INFO -  Operating system: Mac OS X
09:36:31     INFO -                    10.10.5 14F27
09:36:31     INFO -  CPU: amd64
09:36:31     INFO -       family 6 model 69 stepping 1
09:36:31     INFO -       4 CPUs
09:36:31     INFO -  GPU: UNKNOWN
09:36:31     INFO -  Crash reason:  EXC_BAD_ACCESS / KERN_INVALID_ADDRESS
09:36:31     INFO -  Crash address: 0x0
09:36:31     INFO -  Process uptime: 25 seconds
09:36:31     INFO -  Thread 0 (crashed)
09:36:31     INFO -   0  XUL!ProcessExecutableMemory::release() [ProcessExecutableMemory.cpp:033299f2733900eb0da9b5b4d814aea39ce552d4 : 492 + 0x0]
09:36:31     INFO -      rax = 0x0000000000000000   rdx = 0x00007fff75f5c1f8
09:36:31     INFO -      rcx = 0x0000000000000000   rbx = 0x0000000117fb6690
09:36:31     INFO -      rsi = 0x0003c3000003c300   rdi = 0x0003c2000003c303
09:36:31     INFO -      rbp = 0x00007fff50502dd0   rsp = 0x00007fff50502dc0
09:36:31     INFO -       r8 = 0x00007fff50502d70    r9 = 0x00007fff75f86300
09:36:31     INFO -      r10 = 0x0000000000000000   r11 = 0x0000000000000246
09:36:31     INFO -      r12 = 0x000000011860a9d0   r13 = 0x00007fff50502e30
09:36:31     INFO -      r14 = 0x0000000117fcd690   r15 = 0x00007fff50502e08
09:36:31     INFO -      rip = 0x0000000114af2026
09:36:31     INFO -      Found by: given as instruction pointer in context


Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/30ed797c2454b1f5f259f1c26f85bd7a62380ef5

Push of the failures:
https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=033299f2733900eb0da9b5b4d814aea39ce552d4

Comment 9

[Peter Van der Beken [:peterv]]

This triggers a leak in browser/components/extensions/test/xpcshell/test_ext_geckoProfiler_control.js. I have this in a rr recording, and have been poking at it for a while. This looks like an existing problem, that I just triggered because I made sandboxes preserve their binding.

What I think is happening:

- mozilla::ProfilerChild::RecvStop ends up calling into locked_profiler_stop
- locked_profiler_stop calls StopJSSampling on one RegisteredThread
- locked_profiler_stop does not call PollJSSampling on that RegisteredThread, because |registeredThread->Info()->ThreadId() == tid| is false here: https://searchfox.org/mozilla-central/rev/795575287259a370d00518098472eaa5b362bfa7/tools/profiler/core/platform.cpp#2997
- nothing else calls PollJSSampling
- we end up marking things from the js::jit::JitcodeGlobalTable. I think this is because we're relying on PollJSSampling calling GeckoProfilerRuntime::enable, which would clear things from the js::jit::JitcodeGlobalTable.

Markus, you fixed a similar problem in bug 1355566 by making mozilla::ShutdownXPCOM call profiler_clear_js_context. However, that doesn't do anything here because |ActivePS::Exists(lock)| is false here: https://searchfox.org/mozilla-central/rev/795575287259a370d00518098472eaa5b362bfa7/tools/profiler/core/platform.cpp#3467

Do you happen to know who should be calling PollJSSampling or GeckoProfilerRuntime::enable in this case?

Comment 10

[Markus Stange [:mstange]]

(In reply to Peter Van der Beken [:peterv] from comment #9)
> Do you happen to know who should be calling PollJSSampling or
> GeckoProfilerRuntime::enable in this case?

The hope was that there would be an upcoming call to profiler_js_interrupt_callback() (which calls into PollJSSampling() via PollJSSamplingForCurrentThread()). However, if no more JS runs, I guess that means that there will be no more JS interrupt callbacks.

I think this can be fixed by calling TriggerPollJSSamplingOnMainThread() from locked_profiler_stop, similarly to how it's called from locked_profiler_start.

Thanks for debugging this! I think this also explains bug 1442877 (see bug 1442877 comment 2 and bug 1442877 comment 3).

Comment 11

[Markus Stange [:mstange]]

er, bug 1442877 comment 3 and bug 1442877 comment 4, anyway

Comment 12

[Peter Van der Beken [:peterv]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7fe6da2595a3449900957f868a27acfd45a7d5b7

Comment 13

[Peter Van der Beken [:peterv]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b410a9d743086acae0da54c339a06e306d8ca5e6

Comment 14

[Peter Van der Beken [:peterv]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b43693da0f23da175bdb2042d65a4edb44203dfb

Comment 15

[Peter Van der Beken [:peterv]]

Attachment: Leak fix, make sure we call PollJSSampling at least once after stopping profiler

Comment 16

[Peter Van der Beken [:peterv]]

Attachment: Leak fix, set up CC in XPCShell too.

Comment 17

[Markus Stange [:mstange]]

Comment on attachment 8980652 [details] [diff] [review]
Leak fix, make sure we call PollJSSampling at least once after stopping profiler

Review of attachment 8980652 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!

Comment 18

[Peter Van der Beken [:peterv]]

Attachment: Leak fix, declare ServiceWorkerPrivate::mSandbox to the CC.

Comment 19

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8980665 [details] [diff] [review]
Leak fix, declare ServiceWorkerPrivate::mSandbox to the CC.

Review of attachment 8980665 [details] [diff] [review]:
-----------------------------------------------------------------

Ok, but note I have patches to completely remove this sandbox in bug 1462466.

Comment 20

[Peter Van der Beken [:peterv]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=741e73df5dcf6cceaa5c0d2988b986688bbc72ee

Comment 21

[Peter Van der Beken [:peterv]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f5ed451a71cfe08912a64913ab88b46c269124d5
Bug 1146316 - Preserve the wrapper of sandboxes, so that we never try to call WrapObject on them. r=bz.

https://hg.mozilla.org/integration/mozilla-inbound/rev/a833d415b782d3f76366bd5098c1a187730ed020
Bug 1146316 - Remove nsWrapperCache::SetIsNotDOMBinding and IsDOMBinding(). r=bz.

https://hg.mozilla.org/integration/mozilla-inbound/rev/d27ecca6fe612114bd91003d7ceafc364ca62eac
Bug 1146316 - Leak fix, make sure we call PollJSSampling at least once after stopping profiler. r=mstange.

https://hg.mozilla.org/integration/mozilla-inbound/rev/5c390ce65c956c29c028f02cb122529233d13680
Bug 1146316 - Leak fix, set up CC in XPCShell too. r=smaug.

https://hg.mozilla.org/integration/mozilla-inbound/rev/50808ac6967ff99d90173c55023242133f3c1bb7
Bug 1146316 - Leak fix, declare ServiceWorkerPrivate::mSandbox to the CC. r=bkelly.

Comment 22

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/f5ed451a71cf
https://hg.mozilla.org/mozilla-central/rev/a833d415b782
https://hg.mozilla.org/mozilla-central/rev/d27ecca6fe61
https://hg.mozilla.org/mozilla-central/rev/5c390ce65c95
https://hg.mozilla.org/mozilla-central/rev/50808ac6967f