My patches also implement HTMLObjectElement.contentWindow and move ValidityState to Web IDL as bonus points.

This patch crashes in nsObjectLoadingContent::NotifyContentObjectWrapper when calling GetWrappedNativeOfNativeObject (http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsObjectLoadingContent.cpp#2578). I'm not quite sure how to make that correctly work with objects that do not go through xpconnect. Here's a stack of the assertion failure: Assertion failure: IS_WRAPPER_CLASS(js::GetObjectClass(obj)), at /Users/ehsanakhgari/moz/mozilla-central/js/xpconnect/src/xpcpublic.h:81 Program received signal EXC_BAD_ACCESS, Could not access memory. Reason: KERN_INVALID_ADDRESS at address: 0x0000000000000000 IS_WN_WRAPPER_OBJECT (obj=0x1251321c0) at xpcpublic.h:81 81 MOZ_ASSERT(IS_WRAPPER_CLASS(js::GetObjectClass(obj))); (gdb) bt #0 IS_WN_WRAPPER_OBJECT (obj=0x1251321c0) at xpcpublic.h:81 #1 0x0000000102db3b15 in IS_SLIM_WRAPPER_OBJECT (obj=0x1251321c0) at xpcpublic.h:86 #2 0x0000000102db4394 in XPCWrappedNative::GetUsedOnly (ccx=@0x7fff5fbfc320, Object=0x116769480, Scope=0x124c2c300, Interface=0x10c8a9ac0, resultWrapper=0x7fff5fbfc2e0) at /Users/ehsanakhgari/moz/mozilla-central/js/xpconnect/src/XPCWrappedNative.cpp:812 #3 0x0000000102d262f8 in nsXPConnect::GetWrappedNativeOfNativeObject (this=0x10c6722c0, aJSContext=0x124a41600, aScope=0x12512d060, aCOMObj=0x116769480, aIID=@0x105541550, _retval=0x7fff5fbfc488) at /Users/ehsanakhgari/moz/mozilla-central/js/xpconnect/src/nsXPConnect.cpp:1378 #4 0x0000000101eef82d in nsObjectLoadingContent::NotifyContentObjectWrapper (this=0x116769510) at /Users/ehsanakhgari/moz/mozilla-central/content/base/src/nsObjectLoadingContent.cpp:2580 #5 0x0000000101eef248 in nsObjectLoadingContent::InstantiatePluginInstance (this=0x116769510, aIsLoading=false) at /Users/ehsanakhgari/moz/mozilla-central/content/base/src/nsObjectLoadingContent.cpp:756 #6 0x0000000101ef74fe in nsObjectLoadingContent::SyncStartPluginInstance (this=0x116769510) at /Users/ehsanakhgari/moz/mozilla-central/content/base/src/nsObjectLoadingContent.cpp:2359 #7 0x0000000101eec596 in nsAsyncInstantiateEvent::Run (this=0x124c74f00) at /Users/ehsanakhgari/moz/mozilla-central/content/base/src/nsObjectLoadingContent.cpp:129 #8 0x0000000103bb2be6 in nsThread::ProcessNextEvent (this=0x10053ac40, mayWait=false, result=0x7fff5fbfc8d3) at /Users/ehsanakhgari/moz/mozilla-central/xpcom/threads/nsThread.cpp:627 #9 0x0000000103b1712c in NS_ProcessPendingEvents_P (thread=0x10053ac40, timeout=20) at nsThreadUtils.cpp:187 #10 0x000000010341255f in nsBaseAppShell::NativeEventCallback (this=0x10c67f7e0) at /Users/ehsanakhgari/moz/mozilla-central/widget/xpwidgets/nsBaseAppShell.cpp:97 #11 0x00000001033a230c in nsAppShell::ProcessGeckoEvents (aInfo=0x10c67f7e0) at /Users/ehsanakhgari/moz/mozilla-central/widget/cocoa/nsAppShell.mm:387 #12 0x00007fff8f61e101 in __CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION__ () #13 0x00007fff8f61da25 in __CFRunLoopDoSources0 () #14 0x00007fff8f640dc5 in __CFRunLoopRun () #15 0x00007fff8f6406b2 in CFRunLoopRunSpecific () #16 0x00007fff8a25e0a4 in RunCurrentEventLoopInMode () #17 0x00007fff8a25de42 in ReceiveNextEventCommon () #18 0x00007fff8a25dcd3 in BlockUntilNextEventMatchingListInMode () #19 0x00007fff8ea11613 in _DPSNextEvent () #20 0x00007fff8ea10ed2 in -[NSApplication nextEventMatchingMask:untilDate:inMode:dequeue:] () #21 0x00000001033a0ae7 in -[GeckoNSApplication nextEventMatchingMask:untilDate:inMode:dequeue:] (self=0x10c7efd30, _cmd=0x7fff8f23f444, mask=18446744073709551615, expiration=0x422d63c37f00000d, mode=0x7fff7a4911b0, flag=1 '\001') at /Users/ehsanakhgari/moz/mozilla-central/widget/cocoa/nsAppShell.mm:164 #22 0x00007fff8ea08283 in -[NSApplication run] () #23 0x00000001033a2de1 in nsAppShell::Run (this=0x10c67f7e0) at /Users/ehsanakhgari/moz/mozilla-central/widget/cocoa/nsAppShell.mm:741 #24 0x0000000102fec56c in nsAppStartup::Run (this=0x10c664380) at /Users/ehsanakhgari/moz/mozilla-central/toolkit/components/startup/nsAppStartup.cpp:288 #25 0x00000001013dc382 in XREMain::XRE_mainRun (this=0x7fff5fbfe700) at /Users/ehsanakhgari/moz/mozilla-central/toolkit/xre/nsAppRunner.cpp:3823 #26 0x00000001013dcb4f in XREMain::XRE_main (this=0x7fff5fbfe700, argc=6, argv=0x7fff5fbfefb0, aAppData=0x7fff5fbfe938) at /Users/ehsanakhgari/moz/mozilla-central/toolkit/xre/nsAppRunner.cpp:3890 #27 0x00000001013dcf9d in XRE_main (argc=6, argv=0x7fff5fbfefb0, aAppData=0x7fff5fbfe938, aFlags=0) at /Users/ehsanakhgari/moz/mozilla-central/toolkit/xre/nsAppRunner.cpp:4093 #28 0x000000010000202e in do_main (argc=6, argv=0x7fff5fbfefb0, xreDirectory=0x100527500) at /Users/ehsanakhgari/moz/mozilla-central/browser/app/nsBrowserApp.cpp:195 #29 0x000000010000163d in main (argc=6, argv=0x7fff5fbfefb0) at /Users/ehsanakhgari/moz/mozilla-central/browser/app/nsBrowserApp.cpp:388

You can't do GetWrappedNativeOfNativeObject on non-XPConnect objects, since those do not in fact have any sort of nsIXPConnectWrappedNative. The good news is that this thing doesn't care about the XPCWN; it just wants the JSObject for the element. So you can just replace all that jazz with: JSObject* obj = GetWrapper(); if (!obj) { // Nothing to do here if there's no wrapper for mContent. The proto // chain will be fixed appropriately when the wrapper is created. return; }

But the nsIXPConnectWrappedNative* is also passed to the nsHTMLPluginObjElementSH::SetupProtoChain call further down that function. What shall I do with that?

Well, you need a replacement for nsHTMLPluginObjElementSH::SetupProtoChain, right? It needs be called from the relevant WrapNode implementations... And here you'll want to call either SetupProtoChain or the new method, depending on whether IsDOMBinding(), I think. For what it's worth, the only things SetupProtoChain wants out of the XPCWrappedNative are: 1) The canonical prototype for the class. You'll presumably need to add a virtual method for this or something, since the canonical proto is different for different subclasses of nsObjectLoadingContent. 2) Pass to GetPluginInstanceIfSafe, which only wants the wrapper so it can extract an nsIContent from it. We can get the nsIContent directly from the JSObject as needed, for the WebIDL binding case.

(In reply to comment #6) > Well, you need a replacement for nsHTMLPluginObjElementSH::SetupProtoChain, > right? It needs be called from the relevant WrapNode implementations... And > here you'll want to call either SetupProtoChain or the new method, depending on > whether IsDOMBinding(), I think. > > For what it's worth, the only things SetupProtoChain wants out of the > XPCWrappedNative are: > > 1) The canonical prototype for the class. You'll presumably need to add a > virtual method for this or something, since the canonical proto is different > for different subclasses of nsObjectLoadingContent. > > 2) Pass to GetPluginInstanceIfSafe, which only wants the wrapper so it can > extract an nsIContent from it. We can get the nsIContent directly from the > JSObject as needed, for the WebIDL binding case. So, do I need to keep the current code paths, and add alternate ones to work around these two items?

For now, yes, until all things that are using nsHTMLPluginObjElementSH are on WebIDL.

Alright, I got this to a point where I'm crashing when calling ReleaseSliceNow on an nsISupports* which seems to be freed already. I think I'm giving up on this for now -- I really don't have any idea what I'm doing any more... I think we can take part 1 and 2 here for now anyway. Boris, I can also look into ripping out the ValidityState parts and land them separately if you want.

This fixes a friend declaration to satisfy g++.

Comment on attachment 701210 [details] [diff] [review] Part 2: Rename nsDOMValidityState to mozilla::dom::ValidityState Not sure why I didn't set the review flag here...

Comment on attachment 701210 [details] [diff] [review] Part 2: Rename nsDOMValidityState to mozilla::dom::ValidityState r=me

Comment on attachment 698471 [details] [diff] [review] Part 1: Rename nsHTMLObjectElement to mozilla::dom::HTMLObjectElement r=me Please feel free to assign to me once you land the reviewed bits.

So I first landed: https://hg.mozilla.org/integration/mozilla-inbound/rev/a1c95996fc3e https://hg.mozilla.org/integration/mozilla-inbound/rev/4b3fc69a1893 and then immediately realized I forgot to export the patches out of git properly, and therefore did not do proper renaming. :( So I backed out in agony: https://hg.mozilla.org/integration/mozilla-inbound/rev/8c6a29d22334 And then relanded more properly: http://hg.mozilla.org/integration/mozilla-inbound/rev/a28186bf9748 http://hg.mozilla.org/integration/mozilla-inbound/rev/0a8c4b6efd8c

https://tbpl.mozilla.org/?tree=Try&rev=ebb8869e6201 This should be good to go independently of the HTMLObjectElement parts.

This should fix the compiler warning that failed the Windows build.

Comment on attachment 702776 [details] [diff] [review] Part 3: Move ValidityState to Web IDL bindings >+ already_AddRefed<mozilla::dom::ValidityState> Validity(); Why does this need to return already_AddRefed? Can we not just return a ValidityState* and addref only in the case that cares (i.e. the GetValidity method)? r=me with that fixed or explained.

Actually, one more thing. GetHTMLUnsignedIntAttr doesn't really seem related to that patch, but I guess it's not a problem to land it as part of these changes... I assume you'll post whatever your WIP is on <object> itself?

(In reply to Boris Zbarsky (:bz) from comment #18) > Comment on attachment 702776 [details] [diff] [review] > Part 3: Move ValidityState to Web IDL bindings > > >+ already_AddRefed<mozilla::dom::ValidityState> Validity(); > > Why does this need to return already_AddRefed? Can we not just return a > ValidityState* and addref only in the case that cares (i.e. the GetValidity > method)? OK. will do. (In reply to Boris Zbarsky (:bz) from comment #19) > Actually, one more thing. GetHTMLUnsignedIntAttr doesn't really seem > related to that patch, but I guess it's not a problem to land it as part of > these changes... Nah, it's best if I take it out now. I'll put it in the next patch. > I assume you'll post whatever your WIP is on <object> itself? Of course. I was too sleepy last night. :-)

Over to you, Boris!

https://hg.mozilla.org/mozilla-central/rev/a28186bf9748 https://hg.mozilla.org/mozilla-central/rev/0a8c4b6efd8c

Pushed https://hg.mozilla.org/integration/mozilla-inbound/rev/5938f7cbfa8e to traverse the mValidity member of HTMLObjectElement as well.

Comment on attachment 717344 [details] [diff] [review] part 5. Refactor some of the nsHTMLPluginObjElementSH code to be able to work with WebIDL objects. Review of attachment 717344 [details] [diff] [review]: ----------------------------------------------------------------- I'm not terribly familiar with wrapper/js/prototype goop here, but just going from what I do understand about plugin prototypes this looks okay. With the additional of several new DoCrypticPluginJSProtoOrWrapperOperation functions in multiple files, some kind of overview comment for how the whole prototype hackery ties together for plugin tags might be useful (if explaining such a thing briefly is even possible).

Comment on attachment 717352 [details] [diff] [review] part 10. Implement the WebIDL API for <object>. Review of attachment 717352 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/webidl/HTMLObjectElement.webidl @@ +77,5 @@ > + > +[NoInterfaceObject] > +interface MozObjectLoadingContent { > + // Mirrored chrome-only scriptable nsIObjectLoadingContent methods. Please > + // make sure to update this list if nsIObjectLoadingContent changes. How long do these both need to coexist? @@ +175,5 @@ > + [ChromeOnly] > + readonly attribute URI? srcURI; > + > + // It's very important that this stay [ChromeOnly]. If not, the > + // IsCallerChrome check in the implementation will need to be reinstated. Doesn't this apply to almost all the ChromeOnly items here...?

> some kind of overview comment for how the whole prototype hackery ties together for > plugin tags might be useful I can write up something, sure. Note that some of this goop will go away in bug 843627. > How long do these both need to coexist? As long as people are using nsIObjectLoadingContent. After these patches and the ones in bug 843627 JS code can just stop using them; that will leave C++ code. > Doesn't this apply to almost all the ChromeOnly items here...? Most of the rest of them didn't use to prevent untrusted script access, as far as I can tell...

(In reply to Boris Zbarsky (:bz) from comment #38) > > Doesn't this apply to almost all the ChromeOnly items here...? > > Most of the rest of them didn't use to prevent untrusted script access, as > far as I can tell... None of these have any business being exposed to content AFAICT, things like displayedType and getContentTypeForMIMEType are exposed now, but are only useful for probing for more information than navigator.plugins exposes, which we certainly didn't intend. That we tried to block pluginFallbackType but not activated is probably accidental.

> I can write up something, sure. /** * When a plug-in is instantiated, it can create a scriptable * object that the page wants to interact with. We expose this * object by placing it on the prototype chain of our element, * between the element itself and its most-derived DOM prototype. * * GetCanonicalPrototype returns this most-derived DOM prototype. * * SetupProtoChain handles actually inserting the plug-in * scriptable object into the proto chain if needed. * * DoNewResolve is a hook that allows us to find out when the web * page is looking up a property name on our object and make sure * that our plug-in, if any, is instantiated. */

> None of these have any business being exposed to content AFAICT OK, sounds good. The comments for this interface now say: // Mirrored chrome-only scriptable nsIObjectLoadingContent methods. Please // make sure to update this list if nsIObjectLoadingContent changes. Also, // make sure everything on here is [ChromeOnly].

Comment on attachment 717344 [details] [diff] [review] part 5. Refactor some of the nsHTMLPluginObjElementSH code to be able to work with WebIDL objects. Review of attachment 717344 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/base/src/nsObjectLoadingContent.cpp @@ +2765,5 @@ > + // (perhaps because WrapObject can happen on a random compartment?) > + // so make sure to enter the compartment of aObject. > + JSAutoCompartment ac(aCx, aObject); > + MOZ_ASSERT(nsCOMPtr<nsIContent>(do_QueryInterface( > + static_cast<nsIObjectLoadingContent*>(this)))->IsDOMBinding()); do_QueryObject(this) might work. ::: dom/base/nsDOMClassInfo.cpp @@ +8405,5 @@ > } > > +nsHTMLPluginObjElementSH::SetupProtoChainRunner::SetupProtoChainRunner( > + nsIXPConnectWrappedNative* wrapper, > + nsIScriptContext* scriptContext, aWrapper, aScriptContext.

Comment on attachment 717349 [details] [diff] [review] part 8. Implement legacycaller support in WebIDL. Review of attachment 717349 [details] [diff] [review]: ----------------------------------------------------------------- We should throw if we're using a proxy to implement an interface that uses legacycaller, if we're not implementing that (I hacked it up once using |js::SetReservedSlot(obj, js::JSSLOT_PROXY_CALL, JS::NullValue());| on the proxy to force handler's call(...) to be called and then implementing DOMProxyHandler::call). ::: dom/bindings/Codegen.py @@ +157,5 @@ > def declare(self): > return "extern DOMJSClass Class;\n" > def define(self): > traceHook = TRACE_HOOK_NAME if self.descriptor.customTrace else 'NULL' > + callHook = LEGACYCALLER_HOOK_NAME if self.descriptor.operations["LegacyCaller"] else 'NULL' nullptr @@ +7669,5 @@ > + # We already added this method > + return > + if name == "LegacyCaller": > + if op.isIdentifierLess(): > + # XXXbz I wish we were consistent about our renaming here. I'm not sure what these comments are about. ::: dom/bindings/Configuration.py @@ +261,5 @@ > for m in iface.members: > if m.isMethod() and m.isStringifier(): > addOperation('Stringifier', m) > + # Don't worry about inheriting legacycallers either: in > + # practice these are on most-derived prototypes. Do we enforce that somehow? ::: dom/bindings/parser/WebIDL.py @@ +635,5 @@ > self.members.append(unforgeableAttr) > > # Ensure that there's at most one of each {named,indexed} > + # {getter,setter,creator,deleter}, at most one stringifier, > + # and at most one legacycaller. So this deviates from the spec, which seems to allow multiple legacy callers. I'm ok with restricting it, but we should have a comment and probably code in Descriptor.__init__ to throw if it happens.

Comment on attachment 717344 [details] [diff] [review] part 5. Refactor some of the nsHTMLPluginObjElementSH code to be able to work with WebIDL objects. Review of attachment 717344 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/base/src/nsObjectLoadingContent.cpp @@ +2598,5 @@ > + nsHTMLPluginObjElementSH::SetupProtoChain(cx, obj, wrapper, canonicalPrototype); > +} > + > +JSObject* > +nsObjectLoadingContent::GetCanonicalPrototype(JSContext* aCx, JSObject* aGlobal) Btw, you could just use mGetProto from the DOMClass of the JS object instead of this.

> do_QueryObject(this) might work. Sadly, no: there is no QueryInterface declared in nsObjectLoadingContent itself, so the name lookup is ambiguous.... I could define it as pure-virtual if needed, but doesn't seem worth it. > aWrapper, aScriptContext. Done. > We should throw if we're using a proxy to implement an interface that uses > legacycaller, Done, in Configuration.py. > nullptr Done. > I'm not sure what these comments are about. About "LegacyCall" vs "LegacyCaller" and "Stringify" vs "Stringifier".... As in, the fact that we can't use the name of the special op as the method name to call for those. > Do we enforce that somehow? No. I added such enforcement to Configuration.py, just to be safe. Inside the loop that goes up the parent chain in Descriptor.__init__, like so: if m.isLegacycaller() and iface != self.interface: raise TypeError("We don't support legacycaller on " "non-leaf interface %s.\n%s" % (iface, iface.location)) > So this deviates from the spec, which seems to allow multiple legacy callers Indeed. That's never used in practice. I added a comment that this is a spec violation. > and probably code in Descriptor.__init__ to throw if it happens. You mean if our parser is ever aligned with the spec? Done. The other option is to remove support for overloading legacycaller from the spec, which may be a good idea. Posted to public-script-coord about this. > Btw, you could just use mGetProto from the DOMClass of the JS object instead of this. I'll roll this into bug 843627 part 4.

https://hg.mozilla.org/integration/mozilla-inbound/rev/2ed49a3f8c95 https://hg.mozilla.org/integration/mozilla-inbound/rev/6ad7d07de598 https://hg.mozilla.org/integration/mozilla-inbound/rev/6e15e84cf5e4 https://hg.mozilla.org/integration/mozilla-inbound/rev/f108b865d785 https://hg.mozilla.org/integration/mozilla-inbound/rev/b158b0b9d6c5 https://hg.mozilla.org/integration/mozilla-inbound/rev/194ff1cc47b1 https://hg.mozilla.org/integration/mozilla-inbound/rev/2873561bfe40 https://hg.mozilla.org/integration/mozilla-inbound/rev/f22f257f7330

https://hg.mozilla.org/mozilla-central/rev/2ed49a3f8c95 https://hg.mozilla.org/mozilla-central/rev/6ad7d07de598 https://hg.mozilla.org/mozilla-central/rev/6e15e84cf5e4 https://hg.mozilla.org/mozilla-central/rev/f108b865d785 https://hg.mozilla.org/mozilla-central/rev/b158b0b9d6c5 https://hg.mozilla.org/mozilla-central/rev/194ff1cc47b1 https://hg.mozilla.org/mozilla-central/rev/2873561bfe40 https://hg.mozilla.org/mozilla-central/rev/f22f257f7330

The tests are actually in bug 843627 so not landed yet.

Documented legacycaller, the changes to stringifier, and NeedNewResolve at https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings