Closed Bug 827158 Opened 11 years ago Closed 11 years ago

Move HTMLObjectElement to WebIDL

Categories

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

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla22

People

(Reporter: ehsan.akhgari, Assigned: bzbarsky)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete)

Attachments

(12 files, 4 obsolete files)

19.84 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
10.09 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
26.71 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
34.00 KB, patch
Details | Diff | Splinter Review
20.75 KB, patch
peterv
: review+
Details | Diff | Splinter Review
19.51 KB, patch
peterv
: review+
johns
: review+
Details | Diff | Splinter Review
2.40 KB, patch
benjamin
: review+
Details | Diff | Splinter Review
2.04 KB, patch
peterv
: review+
Details | Diff | Splinter Review
26.67 KB, patch
peterv
: review+
Details | Diff | Splinter Review
5.82 KB, patch
peterv
: review+
Details | Diff | Splinter Review
26.02 KB, patch
peterv
: review+
johns
: review+
Details | Diff | Splinter Review
6.11 KB, patch
peterv
: review+
Details | Diff | Splinter Review
My patches also implement HTMLObjectElement.contentWindow and move ValidityState to Web IDL as bonus points.
Assignee: nobody → ehsan
Status: NEW → ASSIGNED
Attachment #698471 - Flags: review?(bzbarsky)
Attachment #698472 - Flags: review?(bzbarsky)
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
Attachment #698493 - Flags: feedback?(bzbarsky)
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.
OS: Mac OS X → All
Hardware: x86 → All
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.
Attachment #698493 - Attachment is obsolete: true
Attachment #698493 - Flags: feedback?(bzbarsky)
This fixes a friend declaration to satisfy g++.
Attachment #698472 - Attachment is obsolete: true
Attachment #698472 - Flags: review?(bzbarsky)
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...
Attachment #701210 - Flags: review?(bzbarsky)
Comment on attachment 701210 [details] [diff] [review]
Part 2: Rename nsDOMValidityState to mozilla::dom::ValidityState

r=me
Attachment #701210 - Flags: review?(bzbarsky) → review+
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.
Attachment #698471 - Flags: review?(bzbarsky) → review+
Whiteboard: [leave open]
https://tbpl.mozilla.org/?tree=Try&rev=ebb8869e6201

This should be good to go independently of the HTMLObjectElement parts.
Attachment #698885 - Attachment is obsolete: true
Attachment #702676 - Flags: review?(bzbarsky)
This should fix the compiler warning that failed the Windows build.
Attachment #702676 - Attachment is obsolete: true
Attachment #702676 - Flags: review?(bzbarsky)
Attachment #702776 - Flags: review?(bzbarsky)
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.
Attachment #702776 - Flags: review?(bzbarsky) → review+
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!
Assignee: ehsan → bzbarsky
Depends on: 837033
Pushed https://hg.mozilla.org/integration/mozilla-inbound/rev/5938f7cbfa8e to traverse the mValidity member of HTMLObjectElement as well.
Blocks: 843627
Keywords: dev-doc-needed
Whiteboard: [leave open] → [leave open][need to document legacycaller and the NeedNewResolve bits]
Depends on: 844322
Attachment #717352 - Flags: review?(peterv)
Attachment #717352 - Flags: review?(jschoenick)
Whiteboard: [leave open][need to document legacycaller and the NeedNewResolve bits] → [need review][need to document legacycaller and the NeedNewResolve bits]
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).
Attachment #717344 - Flags: review?(jschoenick) → review+
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...?
Attachment #717352 - Flags: review?(jschoenick) → review+
> 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].
Blocks: 845248
Attachment #717346 - Flags: review?(benjamin) → review+
Attachment #717342 - Flags: review?(peterv) → review+
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.
Attachment #717344 - Flags: review?(peterv) → review+
Attachment #717348 - Flags: review?(peterv) → review+
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.
Attachment #717349 - Flags: review?(peterv) → review+
Attachment #717350 - Flags: review?(peterv) → review+
Attachment #717352 - Flags: review?(peterv) → review+
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.
Attachment #717353 - Flags: review?(peterv) → review+
> 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.
The tests are actually in bug 843627 so not landed yet.
Flags: in-testsuite+ → in-testsuite?
Documented legacycaller, the changes to stringifier, and NeedNewResolve at https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings
Whiteboard: [need to document legacycaller and the NeedNewResolve bits]
Depends on: 868266
Depends on: 878195
No longer depends on: 878195
Depends on: 883968
Depends on: 889821
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: