Closed
Bug 888600
Opened 11 years ago
Closed 6 years ago
Move ContentFrameMessageManager to WebIDL
Categories
(Core :: DOM: Core & HTML, defect, P2)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla61
Tracking | Status | |
---|---|---|
firefox61 | --- | fixed |
People
(Reporter: Ms2ger, Assigned: peterv)
References
(Depends on 1 open bug, Blocks 3 open bugs)
Details
Attachments
(11 files, 7 obsolete files)
15.38 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
9.13 KB,
text/plain
|
Details | |
20.94 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
60.70 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
87.92 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
79.59 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
131.36 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
3.06 KB,
text/plain
|
Details | |
2.60 KB,
application/x-xpinstall
|
Details | |
6.63 KB,
application/x-xpinstall
|
Details | |
2.36 KB,
patch
|
froydnj
:
review+
Waldo
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Comment 1•10 years ago
|
||
When this is done, the UnwrapArg bits in GlobalObject::GetAsSupports should go away as well.
Comment 2•10 years ago
|
||
Assuming that all our globals will be Web IDL at that point, that is. Maybe that still won't be the case.
Comment 3•10 years ago
|
||
(In reply to Boris Zbarsky [:bz] from comment #2) > Assuming that all our globals will be Web IDL at that point, that is. Maybe > that still won't be the case. There will still be BackstagePasses and Sandboxes. The former could move to WebIDL if that makes things easier. I'm not wild about doing that for the latter.
So is anything blocking this? Or is this just work nobody has done?
Comment 5•10 years ago
|
||
The latter. I'd review patches here in a heartbeat!
Comment 6•10 years ago
|
||
The latter yes. It is surprisingly lots of work to convert all the message manager stuff to use webidl,
Comment 7•10 years ago
|
||
And besides, if anyone wants to fix this, don't dump the reviews to bz. This is my mess and I should either fix this or at least review the patches. :)
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → peterv
Status: NEW → ASSIGNED
Assignee | ||
Comment 8•7 years ago
|
||
Assignee | ||
Comment 9•7 years ago
|
||
Assignee | ||
Comment 10•7 years ago
|
||
Assignee | ||
Comment 11•7 years ago
|
||
Attachment #8905160 -
Attachment is obsolete: true
Updated•7 years ago
|
Priority: -- → P2
Assignee | ||
Comment 12•7 years ago
|
||
Assignee | ||
Comment 13•7 years ago
|
||
Assignee | ||
Comment 14•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=de8d38a9d444ae93d345e09deb427508b85e39c5
Assignee | ||
Comment 15•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=116e13e01bc0a79e88eb8297dcfbcf25c913be8b
Assignee | ||
Comment 16•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=68ecb11011b8522ec3611322f4c6dcc500376f1a
Assignee | ||
Comment 17•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6d940bba27aadda6732fe88895968d009d7837c1
Assignee | ||
Comment 18•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=16a13ff8d6806628fb6a08f0eb583851227f16ed
Assignee | ||
Comment 19•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8692c3b5300ea9d006813c392b41759e785fb5e8
Assignee | ||
Comment 20•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=50bef2fde4bd003fde2f923cd839778070fdb285
Assignee | ||
Comment 21•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1f4201b03c55db0164016ba63e046230bd8e83ac
Assignee | ||
Comment 22•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=285e85209653c56bdab94f1a8b60bcd7e03aa7b7
Assignee | ||
Comment 23•6 years ago
|
||
Attachment #8904915 -
Attachment is obsolete: true
Attachment #8933699 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 24•6 years ago
|
||
Assignee | ||
Comment 25•6 years ago
|
||
Attachment #8905103 -
Attachment is obsolete: true
Attachment #8933707 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 26•6 years ago
|
||
Attachment #8908801 -
Attachment is obsolete: true
Attachment #8933757 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 27•6 years ago
|
||
Attachment #8908802 -
Attachment is obsolete: true
Attachment #8933760 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 28•6 years ago
|
||
Attachment #8908803 -
Attachment is obsolete: true
Attachment #8933762 -
Flags: review?(bzbarsky)
Comment 29•6 years ago
|
||
So exciting! :-)
Assignee | ||
Comment 30•6 years ago
|
||
Attachment #8933763 -
Flags: review?(bzbarsky)
Comment 31•6 years ago
|
||
Comment on attachment 8933699 [details] [diff] [review] Part 1: Add infrastructure to expose 'system' names on a WebIDL global >+class CGSystemBindingInitIds(CGAbstractStaticMethod): Please MOZ_ASSERT mainthread in the impl, since we're using statics. >+ for (uint32_t i = 0; i < ArrayLength(properties); ++i) { Maybe document that we need the index to call IdString()? > class CGResolveSystemBinding(CGAbstractMethod): Assert mainthread, please. >+class CGMayResolveAsSystemBindingName(CGAbstractMethod): Again, assert mainthread. >+ if (!idsInited) { >+ return true; Please document that we can't init ids here because we need to be infallible, but we can just claim that we might resolve anything and the only problem will be a performance hit. That said, we will have that performance hit for all property access on system globals until something calls ResolveSystemBinding or GetSystemBindingNames for the first time. Seems a bit annoying. I wonder whether we can actually just init the ids the first time we create a system global and be done with it. I guess that init could fail, but then we shoud fail system global creation too. >+class CGGetSystemBindingNames(CGAbstractMethod): Please assert mainthread. >+ if (!SystemBindingInitIds(aCx)) { >+ aRv.Throw(NS_ERROR_FAILURE); aRv.NoteJSContextException(); >+ if (!aNames.append(property.id)) { >+ aRv.Throw(NS_ERROR_FAILURE); NS_ERROR_OUT_OF_MEMORY? r=me with those nits
Attachment #8933699 -
Flags: review?(bzbarsky) → review+
Updated•6 years ago
|
Attachment #8933706 -
Attachment mime type: text/x-c++src → text/plain
Comment 32•6 years ago
|
||
Comment on attachment 8933707 [details] [diff] [review] Part 2: Various test fixes to prepare for using WebIDL bindings for MessageManager classes >+++ b/dom/indexedDB/test/head.js >+ }, {once: true, capture: true}, true); This needs documentation. >+++ b/dom/presentation/tests/mochitest/test_presentation_dc_receiver_oop.html > mm.addMessageListener('check-navigator', function checknavigatorHandler(aSuccess) { Why doesn't this need to wrapCallback? I guess the test passes, so it's ok... >+++ b/dom/quota/test/head.js >+ }, {once: true, capture: true}, true); Needs documentation. >+++ b/testing/marionette/proxy.js >- this.mm.removeMessageListener(path, l[1]); >+ this.mm.removeMessageListener(path, l); Um... why is this not causing problems right now?? r=me.
Attachment #8933707 -
Flags: review?(bzbarsky) → review+
Comment 33•6 years ago
|
||
Comment 34•6 years ago
|
||
Comment on attachment 8933757 [details] [diff] [review] Part 3: Add message manager concrete classes for WebIDL >+class ChildProcessMessageManager : public SyncMessageSender Shouldn't this be final? >diff --git a/dom/base/ChromeMessageBroadcaster.cpp b/dom/base/ChromeMessageBroadcaster.cpp >+ChromeMessageBroadcaster::ChromeMessageBroadcaster(nsFrameMessageManager* aParentManager, >+ if (mIsProcessManager) { >+ mozilla::HoldJSObjects(this); So I have to ask: Why is this here and not in the MessageBroadcaster constructor? The old logic does HoldJSObjects any time IsBroadcaster() and mIsProcessManager, right? So why not do it in the place where we know we're a broadcaster? Of course ChromeMessageBroadcaster is the only subclass I see here of MessageBroadcaster, and MessageBroadcaster has a proteted ctor, so there can't be any other MessageBroadcasters.... but then why are these separate classes? >+ // This is a bit hackish. When parent manager is global, we want >+ // to attach the message manager to it immediately. I know you just copied this, but nothing here checks whether the parent manager is global. We're just adding because we're a broadcaster. Again, mabe this should live up in MessageBroadcaster, not ChromeMessageBroadcaster? >+ // Is it just the frame message manager which waits until the >+ // content process is running. I know you just copied this comment.... but it makes no sense. Do you happen to know what it's trying to say? >diff --git a/dom/base/ChromeMessageSender.cpp b/dom/base/ChromeMessageSender.cpp >+ // This is a bit hackish. When parent manager is global, we want >+ // to attach the message manager to it immediately. >+ // Is it just the frame message manager which waits until the >+ // content process is running. Again, the comments, while just copied, are not very understandable. >diff --git a/dom/base/ChromeMessageSender.h b/dom/base/ChromeMessageSender.h >+ ChromeMessageSender(ipc::MessageManagerCallback* aCallback, >+ nsFrameMessageManager* aParentManager, >+ MessageManagerFlags aFlags=MessageManagerFlags::MM_NONE); I have a slight worry here: what if someone passes MM_BROADCASTER to this constructor for flags? Right now no one does, but if someone did we could have a now-impossible situation of a broadcaster not added immediately to its parent, or one that is a process manager but didn't do HoldJSObjects. I suspect what we might want is for the ChromeMessageSender constructor to strip out the MM_BROADCASTER flag from the passed-in flags. >+++ b/dom/base/MessageListenerManager.cpp >+NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION(MessageListenerManager) >+NS_INTERFACE_MAP_END_INHERITING(nsFrameMessageManager) >+NS_IMPL_ADDREF_INHERITED(MessageListenerManager, nsFrameMessageManager) >+NS_IMPL_RELEASE_INHERITED(MessageListenerManager, nsFrameMessageManager) NS_IMPL_ISUPPORTS_CYCLE_COLLECTION_INHERITED_0(MessageListenerManager, nsFrameMessageManager) >+NS_IMPL_CYCLE_COLLECTION_CLASS(MessageListenerManager) >+NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN_INHERITED(MessageListenerManager, >+ nsFrameMessageManager) >+ NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mParentManager) >+NS_IMPL_CYCLE_COLLECTION_TRAVERSE_END >+NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN_INHERITED(MessageListenerManager, >+ nsFrameMessageManager) >+ NS_IMPL_CYCLE_COLLECTION_UNLINK(mParentManager) >+NS_IMPL_CYCLE_COLLECTION_UNLINK_END NS_IMPL_CYCLE_COLLECTION_INHERITED(MessageListenerManager, nsFrameMessageManager, mParentManager) >+++ b/dom/base/nsFrameMessageManager.h >+ friend nsresult NS_NewChildProcessMessageManager(nsISyncMessageSender** aResult); Why does that need to be a friend? It's invoking only public constructors, right? r=me with the above nits picked.
Attachment #8933757 -
Flags: review?(bzbarsky) → review+
Comment 35•6 years ago
|
||
Comment on attachment 8933760 [details] [diff] [review] Part 4: Convert MessageManager to WebIDL >+++ b/dom/base/ChromeMessageBroadcaster.cpp >+ MOZ_ASSERT(xpc::AccessCheck::isChrome(js::GetContextCompartment(aCx))); I'd somewhat prefer: MOZ_ASSERT(nsContentUtils::IsSystemCaller(aCx)); which doesn't involve explicit compartments... >+ // ProcessScriptLoader This should mention what's going on with removeDelayedProcessScript. Presumably the XPCOM signature is OK? >+ void LoadProcessScript(const nsAString& aUrl, bool aAllowDelayedLoad, >+ void GetDelayedProcessScripts(JSContext* aCx, I have to ask. Was there a reason to not put this on the superclass like GetInitialProcessData instead of duplicating these forwarders a few times? (I'll believe that there is; I just want to know it!) >+++ b/dom/base/ChromeMessageSender.cpp >+ MOZ_ASSERT(xpc::AccessCheck::isChrome(js::GetContextCompartment(aCx))); MOZ_ASSERT(nsContentUtils::IsSystemCaller(aCx)); >+++ b/dom/base/ChromeMessageSender.h Same comments here as for ChromeMessageBroadcaster. >+++ b/dom/base/MessageBroadcaster.h Please document that the XPCOM ReleaseCachedProcesses is OK. >+++ b/dom/base/nsFrameMessageManager.cpp >+nsFrameMessageManager::AddMessageListener(const nsAString& aMessageName, >+ MessageListener& aListener, >+ bool aListenWhenClosed, >+ ErrorResult& aError) aError is unused. Can you not just remove it, and the [Throws] annotation in the IDL? >+nsFrameMessageManager::RemoveWeakMessageListener(const nsAString& aMessageName, >+ MessageListener* aListener, So why does this take a nullable arg while RemoveMessageListener takes a non-nullable one, and both Add methods take non-nullable ones? Do we have people doing removeWeakMessageListener(null)? Might be worth documenting in the IDL. > nsFrameMessageManager::LoadScript(const nsAString& aURL, >+ ErrorResult rv; It would be better to sope it to right around the LoadScript call on the child manager, so we never end up passing around an already-failed ErrorResult. And at that point you can just make it IgnoredErrorResult and drop the manual SuppressException() call. >+nsFrameMessageManager::GetDelayedScripts(JSContext* aCx, > JS::Rooted<JSObject*> pair(aCx); This looks unused. Please remove it. >+ url = JS_NewUCStringCopyN(aCx, mPendingScripts[i].get(), >+ mPendingScripts[i].Length()); So... I know that's what the code used to do, but I'd really prefer it if we just used ToJSValue here: JS::Rooted<JS::Value> url(aCx); if (!ToJSValue(aCx, mPendingScripts[i], &url)) { aError.NoteJSContextException(aCx); return; } >+static JSObject* >+CreateJSArrayObject(JSContext* aCx, const nsTArray<JS::Value>& aArray) Again, I'd prefer just using ToJSValue. See below. >+nsresult >+nsFrameMessageManager::GetDelayedScripts(JSContext* aCx, >+ nsTArray<nsTArray<JS::Value>> list; .... >+ JS::Rooted<JSObject*> array(aCx, JS_NewArrayObject(aCx, list.Length())); I think you can remove all this manual array creation and CreateJSArrayObject stuff. Once you have "list" built up: if (!ToJSValue(aCx, list, aList)) { return NS_ERROR_OUT_OF_MEMORY; } return NS_OK; >@@ -619,21 +713,84 @@ nsFrameMessageManager::SendMessage(const >+ JS::Rooted<JSObject*> dataArray(aCx, CreateJSArrayObject(aCx, result)); ToJSValue. >+nsFrameMessageManager::SendMessage(JSContext* aCx, The profiler label bit needs to be either in the shared code or at entry into the webidl version in addition to being at entry into the XPCOM version. The #ifdef FUZZING block from SendMessage seems to be getting lost here. The AllowMessage check is also getting lost, afaict. >@@ -922,219 +1103,210 @@ nsFrameMessageManager::ReceiveMessage(ns >+ if (!object) { >+ continue; This part should be happening in the webidl case too, because ObjectOrNull can return null... >+ argument.mObjects.Construct(cpows); Might be worth just marking this member required in the dictionaty, if this is the only place it comes up. Then you won't need the Optional<> in there and can just do: argument.mObjects = cpows; >- JS::Rooted<JS::Value> json(cx, JS::NullValue()); >+ JS::Rooted<JS::Value> json(cx); Why the change from null to undefined if !aCloneData? Seems fishy. >+ argument.mName.Construct(aMessage); >+ argument.mPrincipal.Construct(aPrincipal); >+ argument.mSync.Construct(aIsSync); >+ argument.mTarget.Construct(aTarget); Again, could make all those "required". The principal behavior is changing to set the property to null, not undefined, when aPrincipal is null. That's probably OK... However the dictionary member claims to not be nullable, which is fishy in this case. It should be nullable. Alternately, you could condition the argument.mPrincipal.Construct() bit on aPrincipal being non-null. Then we would switch from defining with undefined value to not defining at all (which is what's happenign with "ports" too). I _think_ that should be ok too... As far as that goes, is aTarget guaranteed non-null? If not, the "target" dictionary member should be nullable... >+ webIDLListener->ReceiveMessage(thisValue, argument, &rval, rv); You should be holding a strong ref on the stack to webIDLListener while making that call. Nothing guarantees that our member keeping it alive won't get changed by the call... >+ JSAutoCompartment tac(cx, thisObject); Fwiw, given how we set up our AutoEntryScript, I'm 99% sure we're already in that compartment... >+ if (!JS_CallFunctionValue(cx, thisObject, funval, >+ JS::HandleValueArray(argv), &rval)) { > continue; > } Hmm. So yes, your new code for the webidl case matches these semantics. And they're OK, because the AES is scoped inside the loop, so this "continue" has the AES report any currently-pending exception on the JSContext... That part is _not_ obvious; may be worth a comment. >+nsFrameMessageManager::GetInitialProcessData(JSContext* aCx, >+ aInitialProcessData.set(init.toObjectOrNull()); I don't see what guarantees that "init" can't be undefined or whatever here. We should either make this return "any" or throw if it's not object-or-null at this point... >+already_AddRefed<nsIMessageSender> >+nsFrameMessageManager::GetProcessMessageManager(ErrorResult& aError) This never fails. Please remove the Throws annotation. >+++ b/dom/base/nsFrameMessageManager.h >+ // ContentProcessMessageManager >+ void GetInitialProcessData(JSContext* aCx, Comment should say "GlobalProcessScriptLoader", no? >+++ b/dom/bindings/Bindings.conf SyncMessageSender should be 'concrete':False as well, I believe. >+++ b/dom/webidl/MessageManager.webidl >+interface ProcessScriptLoader >+ * Returns all delayed scripts that will be loaded once a (remote) >+ * frame becomes available. The return value is a list of URLs. That comment is wrong (also wrong where you copied it from). It's a list of (url, boolean) pairs, right? The FrameScriptLoader comments get this right. >+ChromeMessageBroadcaster implements GlobalProcessScriptLoader; >+ChromeMessageBroadcaster implements FrameScriptLoader; Ugh. It's insane that it implements both but is expected to only be used by one (and if you try to mix them, weird stuff happens). But I guess the current XPCOM setup has the same insanity due to classinfo. That is, the classinfo exposes interfaces the underlying object claims to not QI to!!! Might be worth a followup bug to clean this up and only expose the things that should be exposed, based on whether mIsProcessManager is true. r=me with the above bits fixed
Attachment #8933760 -
Flags: review?(bzbarsky) → review+
Comment 36•6 years ago
|
||
Hey bz, Bobby says you're getting rid of all the xpcscriptable machinery in xpconnect once this bug lands - is there a bug on file for that? If not, can you let me know when you file one? I'm looking at doing some xpconnect-related performance stuff by looking at how we (ab)use it in the frontend, and might as well wait with some of that until the removal happens, if it's happening soon.
Flags: needinfo?(bzbarsky)
Comment 38•6 years ago
|
||
Comment on attachment 8933762 [details] [diff] [review] Part 5: Convert MessageManager globals to WebIDL >+++ b/devtools/client/netmonitor/test/browser_net_view-source-debugger.js >- let wait = waitForDOM(document, "#stack-trace-panel .frame-link", 4); >+ let wait = waitForDOM(document, "#stack-trace-panel .frame-link", 5); Please explain this change in the commit message. >+++ b/dom/base/ContentFrameMessageManager.h This file could really use a documentation comment explaining what this class if for. >+++ b/dom/base/MessageManagerGlobal.h Again, documentation would be good. >+++ b/dom/base/ProcessGlobal.cpp >- NS_DOM_INTERFACE_MAP_ENTRY_CLASSINFO(ContentProcessMessageManager) Can you remove the relevant bigts from nsDOMClassInfo.cpp and nsDOMClassInfoID.h? I fact, I think you could remove the ChromeMessageSender/Broadcaster bits in the previous patch, and the Content*MessageManager bits here. >+++ b/dom/base/ProcessGlobal.h >+ public mozilla::dom::MessageManagerGlobal, You don't need the "mozilla::dom" here, I think; this code is already in that namespace. >+++ b/dom/base/nsInProcessTabChildGlobal.cpp >+nsInProcessTabChildGlobal::GetTabEventTarget(mozilla::ErrorResult& aError) Why do you need aError here? None of the GetTabEventTarget() implementations are fallible, as far as I can tell. >+++ b/dom/ipc/TabChild.cpp > TabChildGlobal::TabChildGlobal(TabChild* aTabChild) >- SetIsNotDOMBinding(); Do we not need to HoldJSObjects here? Also why do we not need to preserve wrappers for these various globals? Is that handled somewhere else? >TabChildGlobal::Init() Why is this safe to remove? Now we're doing TelemetryScrollProbe::Create in the ctor, when our refcount is 0, which seems very questionable to me. I think we should put the init call back, and take a ref before doing the TelemetryScrollProbe::Create bits like we used to. >+++ b/dom/ipc/TabChild.h >+class TabChildGlobal : public mozilla::dom::ContentFrameMessageManager, No need for "mozilla::dom" here. >+++ b/dom/webidl/MessageManager.webidl >+ readonly attribute nsIEventTarget tabEventTarget; This is guaranteed to be non-null? >+ContentProcessMessageManager implements MessageManagerGlobal; That's a bit unfortunate. MessageManagerGlobal inherits from SyncMessageSender, which is a real interface, not a mixin. This will need to be changed to implemement the mixin changes from webidl... I guess it's OK to land this for now, and fix it when we do mixins, but a comment here about how it will need to be fixed is not a bad idea. And possibly a followup bug blocking bug 1414372. r=me with the above issues (esp. the Init() bit) fixed.
Attachment #8933762 -
Flags: review?(bzbarsky) → review+
Comment 39•6 years ago
|
||
Comment on attachment 8933763 [details] [diff] [review] Part 6: Mark some IDL interfaces as non-scriptable >+++ b/dom/base/nsIMessageManager.idl >+[builtinclass, uuid(bb5d79e4-e73c-45e7-9651-4d718f4b994c)] The "builtinclass" is meaningless for a non-scriptable interface. Same for some other interfaces in this file. >+++ b/toolkit/components/satchel/FormHistoryStartup.js >+ if (message.target instanceof MessageListenerManager) { This isn't ideal, because we'd somewhat like to change the instanceof behavior of WebIDL interfaces at some point, since it looks like no other browser will implement it. Unfortunately, MessageListenerManager is not a leaf interface, so ChromeUtils.getClassName probably won't work here. Maybe just file a followup bug on this bit? r=me with those nits. Thank you very much for doing this, and I'm sorry the reviews were so slow.
Attachment #8933763 -
Flags: review?(bzbarsky) → review+
Assignee | ||
Comment 40•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=37a38f92c251cf5cfd26fe2cab62212e69334f69
Assignee | ||
Comment 41•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5979b76dd6b79002281721c3373f4db6679c2136
Assignee | ||
Comment 42•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0fbf8277a87b93e77e298444e06db9611353d9fb
Assignee | ||
Comment 43•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=865545672a77b7df7a8ba1104a6072e837469161
Assignee | ||
Comment 44•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e57a29e00fe2598652b3a0af6131f90bcc594e84
Assignee | ||
Comment 45•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=510b0668dc4fa3b3be6868ea50acaf0ee20d00de
Assignee | ||
Comment 46•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e6d02d2696e12ea2b900d1d14a00f38f93e848d2
Assignee | ||
Comment 47•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=71475fc972c907aafb35a03283c155ee83c634b2
Assignee | ||
Comment 48•6 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/0949dba396adfcaca5c5d94e2d15203ea62b577e Bug 888600 - Move ContentFrameMessageManager to WebIDL. Part 1: Add infrastructure to expose 'system' names on a WebIDL global. r=bz. https://hg.mozilla.org/integration/mozilla-inbound/rev/a109987af625889aadaed85346e0dcec0e1ea1a9 Bug 888600 - Move ContentFrameMessageManager to WebIDL. Part 2: Various test fixes to prepare for using WebIDL bindings for MessageManager classes. r=bz. https://hg.mozilla.org/integration/mozilla-inbound/rev/16af80d6cd93d194bc9bd496ed29e053bb234602 Bug 888600 - Move ContentFrameMessageManager to WebIDL. Part 3: Add message manager concrete classes for WebIDL. r=bz. https://hg.mozilla.org/integration/mozilla-inbound/rev/9eafe225b8048c24f0878e0d0bcb0e091fe1c5a2 Bug 888600 - Move ContentFrameMessageManager to WebIDL. Part 4: Convert MessageManager to WebIDL. r=bz. https://hg.mozilla.org/integration/mozilla-inbound/rev/5b990735f1ad01f4f15380d55771d0eb0fe597b7 Bug 888600 - Move ContentFrameMessageManager to WebIDL. Part 5: Convert MessageManager globals to WebIDL. r=bz. https://hg.mozilla.org/integration/mozilla-inbound/rev/7c8317f4ad481a3376169f7b35c011c2bb64f520 Bug 888600 - Move ContentFrameMessageManager to WebIDL. Part 6: Mark some IDL interfaces as non-scriptable. r=bz.
Assignee | ||
Comment 49•6 years ago
|
||
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #31) > Comment on attachment 8933699 [details] [diff] [review] > That said, we will have that performance hit for all property access on > system globals until something calls ResolveSystemBinding or > GetSystemBindingNames for the first time. Seems a bit annoying. I wonder > whether we can actually just init the ids the first time we create a system > global and be done with it. I guess that init could fail, but then we shoud > fail system global creation too. I added calls to SystemBindingInitIds in xpc::InitClassesWithNewWrappedGlobal and nsMessageManagerScriptExecutor::InitChildGlobalInternal. (In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #32) > >+++ b/dom/presentation/tests/mochitest/test_presentation_dc_receiver_oop.html > > mm.addMessageListener('check-navigator', function checknavigatorHandler(aSuccess) { > > Why doesn't this need to wrapCallback? IIRC both would work. > >- this.mm.removeMessageListener(path, l[1]); > >+ this.mm.removeMessageListener(path, l); > > Um... why is this not causing problems right now?? I don't know, might be that we ignore the leak? (In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #34) > Of course ChromeMessageBroadcaster is the only subclass I see here of > MessageBroadcaster, and MessageBroadcaster has a proteted ctor, so there > can't be any other MessageBroadcasters.... but then why are these separate > classes? I just merged MessageBroadcaster into ChromeMessageBroadcaster. > > >+ // This is a bit hackish. When parent manager is global, we want > >+ // to attach the message manager to it immediately. > > I know you just copied this, but nothing here checks whether the parent > manager is global. We're just adding because we're a broadcaster. Again, > mabe this should live up in MessageBroadcaster, not ChromeMessageBroadcaster? I removed the comment, it was incorrect (even before I moved it). > >+ // Is it just the frame message manager which waits until the > >+ // content process is running. > > I know you just copied this comment.... but it makes no sense. Do you > happen to know what it's trying to say? Removed the comment, it didn't apply to this case. > >diff --git a/dom/base/ChromeMessageSender.cpp b/dom/base/ChromeMessageSender.cpp > >+ // This is a bit hackish. When parent manager is global, we want > >+ // to attach the message manager to it immediately. > >+ // Is it just the frame message manager which waits until the > >+ // content process is running. > > Again, the comments, while just copied, are not very understandable. Changed it to: // This is a bit hackish. We attach to the parent, but only if we have a callback. We // don't have a callback for the frame message manager, and for parent process message // managers (except the parent in-process message manager). In those cases we wait until // the child process is running (see MessageSender::InitWithCallback). This reflects reality more closely. > >diff --git a/dom/base/ChromeMessageSender.h b/dom/base/ChromeMessageSender.h > >+ ChromeMessageSender(ipc::MessageManagerCallback* aCallback, > >+ nsFrameMessageManager* aParentManager, > >+ MessageManagerFlags aFlags=MessageManagerFlags::MM_NONE); > > I have a slight worry here: what if someone passes MM_BROADCASTER to this > constructor for flags? Right now no one does, but if someone did we could > have a now-impossible situation of a broadcaster not added immediately to > its parent, or one that is a process manager but didn't do HoldJSObjects. > > I suspect what we might want is for the ChromeMessageSender constructor to > strip out the MM_BROADCASTER flag from the passed-in flags. I actually added asserts to public constructors taking flags, like: MOZ_ASSERT(!(aFlags & ~(MessageManagerFlags::MM_GLOBAL | MessageManagerFlags::MM_PROCESSMANAGER | MessageManagerFlags::MM_OWNSCALLBACK))); (In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #35) > >+ void LoadProcessScript(const nsAString& aUrl, bool aAllowDelayedLoad, > >+ void GetDelayedProcessScripts(JSContext* aCx, > > I have to ask. Was there a reason to not put this on the superclass like > GetInitialProcessData instead of duplicating these forwarders a few times? > (I'll believe that there is; I just want to know it!) We could put it there, but I prefer to have the implementation on the right class, where it's actually used. The others have to be on nsFrameMessageManager because the globals forward to that. > >+nsFrameMessageManager::AddMessageListener(const nsAString& aMessageName, > >+ MessageListener& aListener, > >+ bool aListenWhenClosed, > >+ ErrorResult& aError) > > aError is unused. Can you not just remove it, and the [Throws] annotation > in the IDL? The globals can actually throw, and they use the same member. > I think you can remove all this manual array creation and CreateJSArrayObject stuff. > Once you have "list" built up: > > if (!ToJSValue(aCx, list, aList)) { > return NS_ERROR_OUT_OF_MEMORY; > } Had to make some changes to ToJSValue to support this. > As far as that goes, is aTarget guaranteed non-null? If not, the "target" > dictionary member should be nullable... I went through the callers, I think it's guaranteed non-null. > >+already_AddRefed<nsIMessageSender> > >+nsFrameMessageManager::GetProcessMessageManager(ErrorResult& aError) > > This never fails. Please remove the Throws annotation. The globals can actually throw, and they use the same member. (In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #38) > Comment on attachment 8933762 [details] [diff] [review] > Part 5: Convert MessageManager globals to WebIDL > >+++ b/dom/ipc/TabChild.cpp > > TabChildGlobal::TabChildGlobal(TabChild* aTabChild) > >- SetIsNotDOMBinding(); > > Do we not need to HoldJSObjects here? No, I don't think we do, this doesn't hold any JS objects apart from the binding. Took care of the other comments.
Comment 50•6 years ago
|
||
Backed out 6 changesets (bug 888600) for Valgrind and build bustge on a CLOSED TREE Log of the failures: https://treeherder.mozilla.org/logviewer.html#?job_id=163646071&repo=mozilla-inbound&lineNumber=29123 https://treeherder.mozilla.org/logviewer.html#?job_id=163646048&repo=mozilla-inbound&lineNumber=37333 Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/c4de1162741eacc436221facc3444481e6065390 Push that got backed out: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=7c8317f4ad481a3376169f7b35c011c2bb64f520
Flags: needinfo?(peterv)
Assignee | ||
Comment 51•6 years ago
|
||
Sigh. The quiter extension that we use for Valgrind and PGO is checked into the tree as a signed XPI, so when we change Gecko APIs that it uses (like I do here) then we need to get a new signed XPI to check in alongside.
Flags: needinfo?(peterv)
(In reply to Peter Van der Beken [:peterv] from comment #51) > Sigh. The quiter extension that we use for Valgrind and PGO is checked into > the tree as a signed XPI, so when we change Gecko APIs that it uses (like I > do here) then we need to get a new signed XPI to check in alongside. I believe this page has related details and links to signing keys on Mana: https://wiki.mozilla.org/EngineeringProductivity/HowTo/SignExtensions
Assignee | ||
Comment 53•6 years ago
|
||
Jason, this bug updates the code of the quitter extension that is used in testing, and it needs to be signed with "Mozilla Extensions" (see also bug 1393162). Can you please have the xpi from this attachment signed with "Mozilla Extensions" so that I can land it together with my other patches for this bug?
Flags: needinfo?(jthomas)
Assignee | ||
Comment 54•6 years ago
|
||
Hmm, I'll try to do it myself with those instructions.
Flags: needinfo?(jthomas)
Assignee | ||
Comment 55•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=aea28ae66fd7eecc643e9a7cde59801cd3a3b038
Assignee | ||
Comment 56•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0f36fa76ea74abbece62ae853044d863827a8996
Assignee | ||
Comment 57•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=35f92cd8aa147946b087ed3bc194dcc6e4e2cc15
Assignee | ||
Comment 58•6 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/c7bd4c6c9741aca337592b50f8533ef9a30ff28d Bug 888600 - Move ContentFrameMessageManager to WebIDL. Part 1: Add infrastructure to expose 'system' names on a WebIDL global. r=bz. https://hg.mozilla.org/integration/mozilla-inbound/rev/30d568d628dd2be0d8f928905a472a9a27bf2a18 Bug 888600 - Move ContentFrameMessageManager to WebIDL. Part 2: Various test fixes to prepare for using WebIDL bindings for MessageManager classes. r=bz. https://hg.mozilla.org/integration/mozilla-inbound/rev/79ef59047e638311f6249f82b528d6ab5d3a7666 Bug 888600 - Move ContentFrameMessageManager to WebIDL. Part 3: Add message manager concrete classes for WebIDL. r=bz. https://hg.mozilla.org/integration/mozilla-inbound/rev/af53037819615484e614e7343256f31b237f1bab Bug 888600 - Move ContentFrameMessageManager to WebIDL. Part 4: Convert MessageManager to WebIDL. r=bz. https://hg.mozilla.org/integration/mozilla-inbound/rev/2efb9b1753f63748fa547117c0665e8f3e705e08 Bug 888600 - Move ContentFrameMessageManager to WebIDL. Part 5: Convert MessageManager globals to WebIDL. r=bz. https://hg.mozilla.org/integration/mozilla-inbound/rev/83c87140dc3da6d521c9380beea85b30cb69c7b5 Bug 888600 - Move ContentFrameMessageManager to WebIDL. Part 6: Mark some IDL interfaces as non-scriptable. r=bz.
Comment 59•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/c7bd4c6c9741 https://hg.mozilla.org/mozilla-central/rev/30d568d628dd https://hg.mozilla.org/mozilla-central/rev/79ef59047e63 https://hg.mozilla.org/mozilla-central/rev/af5303781961 https://hg.mozilla.org/mozilla-central/rev/2efb9b1753f6 https://hg.mozilla.org/mozilla-central/rev/83c87140dc3d
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox60:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Comment 60•6 years ago
|
||
This improved performance according to our tabpaint test! I noticed this gain showed up on Windows platforms also but only for a short while. Another bug shadowed the improvement from Windows. == Change summary for alert #11805 (as of Tue, 27 Feb 2018 09:35:06 GMT) == Improvements: 4% tabpaint linux64 pgo e10s stylo 51.94 -> 49.65 For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=11805
Comment 61•6 years ago
|
||
Backed out 6 changesets (bug 888600) for beta simulation failures: build bustage on Linux and Windows opt (bug 1442036) and devtools failure browser_net_view-source-debugger.js (bug 1441961): https://hg.mozilla.org/mozilla-central/rev/714ff834425c99e5521b7b0e3c3c736bf2f34916
Status: RESOLVED → REOPENED
status-firefox60:
fixed → ---
Flags: needinfo?(peterv)
Resolution: FIXED → ---
Target Milestone: mozilla60 → ---
Comment 62•6 years ago
|
||
Note that while working on bug 1440321 (converting Task.jsm to async/await in devtools), I'm also getting failures on this test. It may just be racy... I'll report back here once I narrow it down on the Task.jsm front.
Comment 63•6 years ago
|
||
Before the backout from comment 61, we noticed these regressions. Please address them before relanding. == Change summary for alert #11858 (as of Thu, 01 Mar 2018 09:21:33 GMT) == Regressions: 1% compiler_metrics num_static_constructors linux32 pgo 134.00 -> 136.00 1% compiler_metrics num_static_constructors linux64 pgo 134.00 -> 136.00 1% compiler_metrics num_static_constructors linux32 opt 134.00 -> 136.00 0% compiler_metrics num_static_constructors linux32 debug 245.00 -> 246.00 For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=11858
Comment 64•6 years ago
|
||
Ionut, is there any data on what the new static constructors are, exactly? That would make that regression a lot easier to address...
Flags: needinfo?(igoldan)
Assignee | ||
Comment 65•6 years ago
|
||
The Valgrind build log has "addons.xpi WARN Add-on quitter@mozilla.org is not correctly signed.", so it seems the signing procedure from https://wiki.mozilla.org/EngineeringProductivity/HowTo/SignExtensions doesn't lead to an xpi usable on beta.
Comment 67•6 years ago
|
||
(In reply to Peter Van der Beken [:peterv] from comment #65) > The Valgrind build log has "addons.xpi WARN Add-on quitter@mozilla.org is > not correctly signed.", so it seems the signing procedure from > https://wiki.mozilla.org/EngineeringProductivity/HowTo/SignExtensions > doesn't lead to an xpi usable on beta. https://mana.mozilla.org/wiki/display/FIREFOX/Internal+Extension+Signing is the documentation for signing Internal Extensions. I've ni'd :wezhou to assist.
Flags: needinfo?(jthomas) → needinfo?(wezhou)
Assignee | ||
Comment 68•6 years ago
|
||
Comment on attachment 8953110 [details] quitter@mozilla.org.xpi This is not the right version.
Attachment #8953110 -
Attachment is obsolete: true
Assignee | ||
Comment 69•6 years ago
|
||
Hmm, the xpi I checked in doesn't contain the META-INF directory. No idea why that worked on trunk.
(In reply to Peter Van der Beken [:peterv] from comment #69) > Hmm, the xpi I checked in doesn't contain the META-INF directory. No idea > why that worked on trunk. In my experience, it's quite hard to follow how these in-tree signed extensions are working... There's several different docs from different points in time. Some of them only check the signatures on specific release branches, so things can look okay during development but then blow up later. Overall, it's a frustrating workflow... If we at least had one clear document that covered all of them, with clear steps to take when making changes to each, that would be great progress. (For example, this quitter add-on doesn't appear on https://mana.mozilla.org/wiki/display/FIREFOX/Internal+Extension+Signing, so it's unclear if that's really the appropriate mechanism.)
Assignee | ||
Comment 71•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2f12dd61fa1bda04fc20cc7f565d95cc87d7ce7f
Comment 72•6 years ago
|
||
I'm not sure what info is needed from me at this point. If you still do, please let me know.
Flags: needinfo?(wezhou)
Assignee | ||
Comment 73•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=09c70ab4ba89d773a23e4252aa62ba03df5b2b9f
Comment 74•6 years ago
|
||
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #64) > Ionut, is there any data on what the new static constructors are, exactly? > That would make that regression a lot easier to address... I don't know more about this test, so I'm asking the owner.
Flags: needinfo?(igoldan) → needinfo?(nfroyd)
Assignee | ||
Comment 75•6 years ago
|
||
Looks like they're coming from ResolveSystemBinding.cpp, should be able to fix that.
Flags: needinfo?(nfroyd)
Assignee | ||
Comment 76•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b6fc0e551a54e6444b61d2aef1ffffafd5ebb11d
Assignee | ||
Comment 77•6 years ago
|
||
The devtools failure is because on beta and release we set javascript.options.asyncstack to false, so there's one less stackframe.
Assignee | ||
Comment 78•6 years ago
|
||
Assignee | ||
Comment 79•6 years ago
|
||
(In reply to :wezhou from comment #72) > I'm not sure what info is needed from me at this point. If you still do, > please let me know. What we need is the quitter@mozilla.org.xpi signed by "Mozilla Extensions" (see bug 1393162 when this was last done). I've attached the xpi to be signed in attachment 8955474 [details].
Flags: needinfo?(peterv) → needinfo?(wezhou)
Comment 80•6 years ago
|
||
What kind of addon is quitter@mozilla.org.xpi? The kind of addons I can help sign is referred to as "system addons", and they will be signed by the following issuer: > Issuer: C=US, O=Mozilla Corporation, OU=Mozilla AMO Production Signing Service, CN=production-signing-ca.addons.mozilla.org/emailAddress=services-ops+addonsigning@mozilla.com Is this the same as signed by "Mozilla Extensions"? If the said addon is what known as "internal addons", then I believe the process to sign them is in these documents: * https://mana.mozilla.org/wiki/display/FIREFOX/Internal+Extension+Signing * https://mana.mozilla.org/wiki/display/SVCOPS/Sign+a+Mozilla+Internal+Extension
Flags: needinfo?(wezhou)
Since Jason was the one who signed the last version (in bug 1393162), maybe he knows the right procedure here?
Flags: needinfo?(jthomas)
Comment 82•6 years ago
|
||
"Mozilla Extensions" aka Internal Extensions signing is self service. Who ever is the owner of the quitter@mozilla.org.xpi should request access to sign these sort of add-ons using the process here https://mana.mozilla.org/wiki/display/FIREFOX/Internal+Extension+Signing. If you need this add-on signed right away then :wezhou can sign it for you.
Flags: needinfo?(jthomas)
Assignee | ||
Comment 83•6 years ago
|
||
Ted, are you the right person to be the owner of the quitter extension? (see comment 82)
Flags: needinfo?(ted)
Assignee | ||
Comment 85•6 years ago
|
||
I don't think these patches should be blocked on finding an owner for that code.
:wezhou, can you sign the extension from attachment 8955474 [details] with "Mozilla Extensions"? Thanks!
Flags: needinfo?(wezhou)
Assignee | ||
Comment 86•6 years ago
|
||
I filed bug 1442741 on finding an owner.
Comment 87•6 years ago
|
||
Perter, do you want it to be signed by -stage or -prod environment? Please see https://mana.mozilla.org/wiki/display/SVCOPS/Sign+a+Mozilla+Internal+Extension#SignaMozillaInternalExtension-Environments for the difference.
Flags: needinfo?(wezhou)
Assignee | ||
Comment 88•6 years ago
|
||
I'm assuming -prod, this needs to run on automation in builds we release. The current version in the tree has: "issuer: C=US, O=Mozilla Corporation, OU=Mozilla AMO Production Signing Service, CN=signingca1.addons.mozilla.org/emailAddress=foxsec@mozilla.com"
Comment 89•6 years ago
|
||
Hi Peter, Please see attached for the signed file. As mentioned above, "Mozilla Extensions" aka Internal Extensions signing is self service. If you often need to sign this kind of extensions, I encourage you to apply for one account to be able to sign them. Reading the doc [1], I don't think it *must* be the extension owners who can apply. [1] https://mana.mozilla.org/wiki/display/FIREFOX/Internal+Extension+Signing
Assignee | ||
Comment 90•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6b1113f58d96fe1e7f9767f554dd229905279bfe
Assignee | ||
Comment 91•6 years ago
|
||
This removes the static constructor that's added when you store a PinnedStringId in a static array.
Attachment #8957292 -
Flags: review?(jwalden+bmo)
Comment 92•6 years ago
|
||
Comment on attachment 8957292 [details] [diff] [review] Part 1a: Make PinnedStringId constructor constexpr Review of attachment 8957292 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/public/Id.h @@ +155,5 @@ > { > return (size_t)JSID_BITS(id) == JSID_TYPE_SYMBOL; > } > > +constexpr JS_PUBLIC_DATA(const jsid) JSID_VOID = { size_t(JSID_TYPE_VOID) }; I...think this is okay, and will not result in umpteen copies of a JSID_VOID symbol separately allocated, as long as people aren't taking the address of JSID_VOID. Which they do not, now. But I'm not 100% certain of that; these aspects of C++ are extraordinarily far from my strong suit. Forwarding review for a second eye. Safest thing would be to, instead of JSID_VOID, have namespace JS { constexpr jsid MakeVoidId() { return jsid { size_t(JSID_TYPE_VOID) }; } } // namespace JS #define JSID_VOID (JS::MakeVoidId()) (and ideally remove JS::MakeVoidId as followup), but if it isn't necessary, it's not worth the hassle.
Attachment #8957292 -
Flags: review?(nfroyd)
Attachment #8957292 -
Flags: review?(jwalden+bmo)
Attachment #8957292 -
Flags: review+
Comment 93•6 years ago
|
||
Comment on attachment 8957292 [details] [diff] [review] Part 1a: Make PinnedStringId constructor constexpr Review of attachment 8957292 [details] [diff] [review]: ----------------------------------------------------------------- This is fine. Waldo's solution of having a constexpr function would also make it difficult for people to depend on address identity of JSID_VOID, which seems like a good thing. Followup? It also seems reasonably to apply the same treatment to the other JSID_* constants. Good followup?
Attachment #8957292 -
Flags: review?(nfroyd) → review+
Assignee | ||
Comment 94•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0ee3ef3aa0bb1623d4a940623069691473801bca
Assignee | ||
Comment 95•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=42b2bc4124bd1cb381e0c1584606523ae33a8cfe
Assignee | ||
Comment 96•6 years ago
|
||
I had to remove the JS_PUBLIC_DATA for JSID_VOID. And there's now still a problem with generating the binding for JSID_VOID for Rust.
Comment 97•6 years ago
|
||
(In reply to Peter Van der Beken [:peterv] from comment #96) > I had to remove the JS_PUBLIC_DATA for JSID_VOID. And there's now still a > problem with generating the binding for JSID_VOID for Rust. The SM Rust build is tier 2 and this seems like an important bug, so landing + a follow-up bug would be acceptable IMO. Moving the following line to WHITELIST_FUNCTIONS might help: https://searchfox.org/mozilla-central/rev/877c99c523a054419ec964d4dfb3f0cadac9d497/js/rust/build.rs#248
Assignee | ||
Comment 98•6 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/a3a9ad856fc9be5962d7d275c1123cfef848086d Bug 888600 - Move ContentFrameMessageManager to WebIDL. Part 1: Add infrastructure to expose 'system' names on a WebIDL global. r=bz. https://hg.mozilla.org/integration/mozilla-inbound/rev/4de6aa3e7bff20c247d2f482cc9d53a9b7e998f8 Bug 888600 - Move ContentFrameMessageManager to WebIDL. Part 1a: Make PinnedStringId constructor constexpr. r=Waldo/froydnj. https://hg.mozilla.org/integration/mozilla-inbound/rev/ddffc18a48a16458d3cc25996469824b19ead16d Bug 888600 - Move ContentFrameMessageManager to WebIDL. Part 2: Various test fixes to prepare for using WebIDL bindings for MessageManager classes. r=bz. https://hg.mozilla.org/integration/mozilla-inbound/rev/00cee09c56f50994d4b5c97cdfc270d80a15645d Bug 888600 - Move ContentFrameMessageManager to WebIDL. Part 3: Add message manager concrete classes for WebIDL. r=bz. https://hg.mozilla.org/integration/mozilla-inbound/rev/a784023ff96061acd91b51b42e4f713d48caab49 Bug 888600 - Move ContentFrameMessageManager to WebIDL. Part 4: Convert MessageManager to WebIDL. r=bz. https://hg.mozilla.org/integration/mozilla-inbound/rev/187b9827d11a6c89a6606ec13bc08e584b6eefe6 Bug 888600 - Move ContentFrameMessageManager to WebIDL. Part 5: Convert MessageManager globals to WebIDL. r=bz. https://hg.mozilla.org/integration/mozilla-inbound/rev/243246b2f4406f0b2f118758220db5c7fdd215df Bug 888600 - Move ContentFrameMessageManager to WebIDL. Part 6: Mark some IDL interfaces as non-scriptable. r=bz.
Comment 99•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/a3a9ad856fc9 https://hg.mozilla.org/mozilla-central/rev/4de6aa3e7bff https://hg.mozilla.org/mozilla-central/rev/ddffc18a48a1 https://hg.mozilla.org/mozilla-central/rev/00cee09c56f5 https://hg.mozilla.org/mozilla-central/rev/a784023ff960 https://hg.mozilla.org/mozilla-central/rev/187b9827d11a https://hg.mozilla.org/mozilla-central/rev/243246b2f440
Status: REOPENED → RESOLVED
Closed: 6 years ago → 6 years ago
status-firefox61:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Comment 100•6 years ago
|
||
It looks like this caused a 151K regresssion in libxul size. https://treeherder.mozilla.org/perf.html#/graphs?timerange=7776000&series=autoland,1338582,0,2&series=mozilla-inbound,1299711,1,2&zoom=1519622212079.0208,1520304288236.8694,121656334.52323402,122479857.16060677&selected=mozilla-inbound,1299711,310434,420153959,2
Comment 101•6 years ago
|
||
Sorry, actually 174k when it relanded. https://treeherder.mozilla.org/perf.html#/graphs?timerange=7776000&series=autoland,1338582,0,2&series=mozilla-inbound,1299711,1,2&zoom=1521641288274.5193,1521651035046.3013,130001619.21093707,130444349.3803149&selected=mozilla-inbound,1299711,318657,435175113,2
Comment 102•6 years ago
|
||
Note that this bug added some duplication, of which 30k was then removed in bug 1448850. And there were also various other cleanups blocked by this bug that removed things. But fundamentally, it looks like this change added ~100k .text, ~20k .rela.dyn, 8k .rodata, 8k .eh_frame, 11k .data.rel.ro, and some minor bits in .data, .bss, .eh_frame_hdr. Looking over the actual symbols, we have lots of code in the binding files... For example, we have two copies of everything in MessageManagerGlobal and the things it inherits from, because it's implemented by two different interfaces (ContentFrameMessageManager and ContentProcessMessageManager). And these functions are not tiny, unfortunately...
Comment 103•6 years ago
|
||
If there ever was a bug to blow some codesize on, this is probably it.
Comment 104•6 years ago
|
||
True, but it's worth thinking a bit about patterns here that make the codesize bigger than it "should" be and addressing them; it might help other cases too.
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•