843627 - Move HTMLEmbedElement and HTMLAppletElement to WebIDL

Status: RESOLVED FIXED

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

Description

[Boris Zbarsky [:bzbarsky]]

Want to do this before pushing <object> so I can sort out all the issues in plugin helper stuff that our test suite only tests with <embed>.

Comment 1

[Boris Zbarsky [:bzbarsky]]

For example, dom/plugins/test/mochitest/test_bug771202.html

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Rename nsHTMLSharedObjectElement to HTMLSharedObjectElement.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Implement the WebIDL API for <embed> and <applet>.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: part 3. Enable WebIDL bindings for <embed> and <applet>.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: part 4. Rip out nsHTMLPluginObjElementSH.

Comment 6

[John Schoenick [:johns]]

Comment on attachment 717360 [details] [diff] [review]
part 4.  Rip out nsHTMLPluginObjElementSH.

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

LGTM aside from possible re-entrance insanity (which probably already exists) (but I'm not a very good authority on js goop)

::: content/base/src/nsObjectLoadingContent.cpp
@@ +2595,5 @@
>      // chain will be fixed appropriately when the wrapper is created.
>      return;
>    }
>  
> +  SetupProtoChain(cx, obj);

Is there any use in having NotifyContentObjectWrapper and SetupProtoChain as separate functions now?

@@ +2860,5 @@
>    return retval;
>  }
>  
>  void
>  nsObjectLoadingContent::SetupProtoChain(JSContext* aCx, JSObject* aObject)

The re-entry insanity in this class also means that we could conceivably queue a SetupProtoChainRunner, then spawn a different plugin, then call SetupProtoChain on that plugin, then hit the SetupProtoChainRunner::Run.

We should probably NS_ENSURE_TRUE(mType == eType_Plugin && mInstanceOwner && !AlreadySetupPrototype, NS_OK)

@@ +2871,5 @@
> +    nsCOMPtr<nsIScriptContext> scriptContext = GetScriptContextFromJSContext(aCx);
> +
> +    nsRefPtr<SetupProtoChainRunner> runner =
> +      new SetupProtoChainRunner(scriptContext, this);
> +    nsContentUtils::AddScriptRunner(runner);

Did you mean to return here?

@@ +2887,4 @@
>    JSAutoCompartment ac(aCx, aObject);
> +
> +  nsRefPtr<nsNPAPIPluginInstance> pi;
> +  nsresult rv = ScriptRequestPluginInstance(aCx, getter_AddRefs(pi));

This should just be |if (mInstanceOwner) mInstanceOwner->GetInstance| -- ScriptRequestPluginInstance tries to spawn an instance if required, but that does not make sense as we only call SetupProtoChain after spawning an instance.

(Spawning instances is also re-entrant)

@@ +2892,5 @@
> +    return;
> +  }
> +
> +  if (!pi) {
> +    // No plugin around for this object.

If we add a check above for not trying to run this function without being in running-plugin state, this should also assert

@@ +3021,5 @@
> +{
> +}
> +
> +NS_IMETHODIMP
> +nsObjectLoadingContent::SetupProtoChainRunner::Run()

Will this cause problems if something has dropped in beneath us and called this?

Comment 7

[Boris Zbarsky [:bzbarsky]]

> Is there any use in having NotifyContentObjectWrapper and SetupProtoChain as separate
> functions now?

Well, they do slightly different things, an the latter is called from all sorts of places.

> The re-entry insanity in this class also means that we could conceivably queue a
> SetupProtoChainRunner, then spawn a different plugin, then call SetupProtoChain on that
> plugin, then hit the SetupProtoChainRunner::Run.

Hmm.  Can we?  Note that it's a scriptrunner, not an event loop runnable....  But maybe that's where the reentry bit matters?  Can we spawn a different plug-in without going into safe-to-run-scripts state first?

> We should probably NS_ENSURE_TRUE(mType == eType_Plugin && mInstanceOwner &&
> !AlreadySetupPrototype, NS_OK)

Looks to me like SetupProtoChain is idempotent as long as our plugin instance doesn't change, and if the plugin instance goes away becomes a no-op.  Is that enough for that AlreadySetup bit?

I did add a check up front in SetupProtoChain to bail if mType != eType_Plugin || !mInstanceOwner.

> Did you mean to return here?

Yes!  Good catch.

> This should just be |if (mInstanceOwner) mInstanceOwner->GetInstance| -- 

Done.

> If we add a check above for not trying to run this function without being in running-
> plugin state, this should also assert

Done.

> Will this cause problems if something has dropped in beneath us and called this?

I don't think it should.

Comment 8

[Boris Zbarsky [:bzbarsky]]

> This should just be |if (mInstanceOwner) mInstanceOwner->GetInstance| --
> ScriptRequestPluginInstance tries to spawn an instance if required, but that does not
> make sense as we only call SetupProtoChain after spawning an instance.

Making this change makes tests fail, because the premise about when SetupProtoChain is called is false: it's also called when JS-wrapping the object, at which point we may not have an instance yet.  So I'm reverting this change, as well as the early bail if !mInstanceOwner (but keeping the early return on mType != eTypePlugin).  I think this means I also have to remove the "have a plugin instance" assert after we call ScriptRequestPluginInstance.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 4 updated to review comments from the HTMLObjectElement bug

Comment 10

[John Schoenick [:johns]]

(In reply to Boris Zbarsky (:bz) from comment #8)
> > This should just be |if (mInstanceOwner) mInstanceOwner->GetInstance| --
> > ScriptRequestPluginInstance tries to spawn an instance if required, but that does not
> > make sense as we only call SetupProtoChain after spawning an instance.
> 
> Making this change makes tests fail, because the premise about when
> SetupProtoChain is called is false: it's also called when JS-wrapping the
> object, at which point we may not have an instance yet.  So I'm reverting
> this change, as well as the early bail if !mInstanceOwner (but keeping the
> early return on mType != eTypePlugin).  I think this means I also have to
> remove the "have a plugin instance" assert after we call
> ScriptRequestPluginInstance.

Alright, this seems fine in that case.(In reply to Boris Zbarsky (:bz) from comment #7)

> > The re-entry insanity in this class also means that we could conceivably queue a
> > SetupProtoChainRunner, then spawn a different plugin, then call SetupProtoChain on that
> > plugin, then hit the SetupProtoChainRunner::Run.
> 
> Hmm.  Can we?  Note that it's a scriptrunner, not an event loop runnable....
> But maybe that's where the reentry bit matters?  Can we spawn a different
> plug-in without going into safe-to-run-scripts state first?

I honestly don't even know. I've fixed numerous re-entry issues in this class, and plugins can cause the most bizarre behaviors. If this is idempotent the worst that can happen is we spawn the plugin early when it wouldn't otherwise be necessary, but that's not the end of the world.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Attachment: part 4. Add IDL attribute reflection tests for <embed>, <applet>, <object>.

Comment 12

[Peter Van der Beken [:peterv]]

Comment on attachment 720801 [details] [diff] [review]
Part 4 updated to review comments from the HTMLObjectElement bug

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

::: content/base/src/nsObjectLoadingContent.cpp
@@ +2830,5 @@
> +    return JS::UndefinedValue();
> +  }
> +
> +  JSObject *pi_obj = nullptr;
> +  JSObject *pi_proto = nullptr;

GetPluginJSObject already nulls.

@@ +2896,5 @@
> +    return;
> +  }
> +
> +  JSObject *pi_obj = nullptr; // XPConnect-wrapped peer object, when we get it.
> +  JSObject *pi_proto = nullptr; // 'pi.__proto__'

GetPluginJSObject already nulls.

@@ +2947,5 @@
> +  //   |__ WebIDL object
> +  //
> +  // pi.__proto__
> +  // ^      ^
> +  // |      |__ Object.prototype

pi.proto could also be something else, right?

Comment 13

[Boris Zbarsky [:bzbarsky]]

> GetPluginJSObject already nulls.

Will fix.

> pi.proto could also be something else, right?

Yeah.  I just copied the comment, but it clearly didn't get updated enough.  I'll expand it out some more and make it correct in the process.

Comment 14

[Boris Zbarsky [:bzbarsky]]

I'm spinning out the "remove classinfo" patch to a separate bug, per discussion.  Pushed the other four:

   https://hg.mozilla.org/integration/mozilla-inbound/rev/d31cfe6eeb24
   https://hg.mozilla.org/integration/mozilla-inbound/rev/61099fdb7b23
   https://hg.mozilla.org/integration/mozilla-inbound/rev/4f79d9d9d818
   https://hg.mozilla.org/integration/mozilla-inbound/rev/ca91f19a36db

Comment 15

[Boris Zbarsky [:bzbarsky]]

Filed bug 851917 on the classinfo removal.

Comment 16

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/d31cfe6eeb24
https://hg.mozilla.org/mozilla-central/rev/61099fdb7b23
https://hg.mozilla.org/mozilla-central/rev/4f79d9d9d818
https://hg.mozilla.org/mozilla-central/rev/ca91f19a36db