Remove nsWrapperCache::SetIsNotDOMBinding and IsDOMBinding()

RESOLVED FIXED in Firefox 62

Status

()

RESOLVED FIXED
3 years ago
3 months ago

People

(Reporter: bzbarsky, Assigned: peterv)

Tracking

(Depends on: 1 bug, Blocks: 1 bug)

Trunk
mozilla62
x86
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox62 fixed)

Details

Attachments

(5 attachments, 1 obsolete attachment)

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

Updated

5 months ago
Assignee: nobody → peterv
Status: NEW → ASSIGNED
(Assignee)

Comment 2

4 months ago
Created attachment 8967074 [details] [diff] [review]
Preserve the wrapper of sandboxes, so that we never try to call WrapObject on them v1
Attachment #8965660 - Attachment is obsolete: true
Attachment #8967074 - Flags: review?(bzbarsky)
(Assignee)

Comment 3

4 months ago
Created attachment 8967076 [details] [diff] [review]
Remove nsWrapperCache::SetIsNotDOMBinding and IsDOMBinding() v2
Attachment #8967076 - Flags: review?(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
Attachment #8967074 - Flags: review?(bzbarsky) → review+
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!
Attachment #8967076 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 6

4 months ago
(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.
(Assignee)

Comment 7

4 months ago
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

4 months ago
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
(Assignee)

Comment 9

4 months ago
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?
Flags: needinfo?(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).
Flags: needinfo?(mstange)
(Assignee)

Comment 15

3 months ago
Created attachment 8980652 [details] [diff] [review]
Leak fix, make sure we call PollJSSampling at least once after stopping profiler
Attachment #8980652 - Flags: review?(mstange)
(Assignee)

Comment 16

3 months ago
Created attachment 8980663 [details] [diff] [review]
Leak fix, set up CC in XPCShell too.
Attachment #8980663 - Flags: review?(bugs)
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!
Attachment #8980652 - Flags: review?(mstange) → review+
(Assignee)

Comment 18

3 months ago
Created attachment 8980665 [details] [diff] [review]
Leak fix, declare ServiceWorkerPrivate::mSandbox to the CC.
Attachment #8980665 - Flags: review?(bkelly)
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.
Attachment #8980665 - Flags: review?(bkelly) → review+

Updated

3 months ago
Attachment #8980663 - Flags: review?(bugs) → review+
(Assignee)

Comment 21

3 months ago
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.
You need to log in before you can comment on or make changes to this bug.