Closed Bug 843627 Opened 7 years ago Closed 7 years ago

Move HTMLEmbedElement and HTMLAppletElement to WebIDL

Categories

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

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla22

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

(Blocks 1 open bug)

Details

Attachments

(5 files, 1 obsolete file)

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>.
For example, dom/plugins/test/mochitest/test_bug771202.html
Attachment #717360 - Flags: review?(peterv)
Attachment #717360 - Flags: review?(jschoenick)
Whiteboard: [need review]
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?
Attachment #717360 - Flags: review?(jschoenick) → review+
> 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.
Flags: needinfo?(jschoenick)
> 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.
Attachment #717360 - Attachment is obsolete: true
Attachment #717360 - Flags: review?(peterv)
(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.
Flags: needinfo?(jschoenick)
Attachment #717354 - Flags: review?(peterv) → review+
Attachment #717357 - Flags: review?(peterv) → review+
Attachment #717359 - Flags: review?(peterv) → review+
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?
Attachment #720801 - Flags: review?(peterv) → review+
Attachment #720828 - Flags: review?(peterv) → review+
> 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.
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
Flags: in-testsuite+
Summary: Move HTMLEmbedElement to WebIDL → Move HTMLEmbedElement and HTMLAppledElement to WebIDL
Whiteboard: [need review]
Target Milestone: --- → mozilla22
Filed bug 851917 on the classinfo removal.
Blocks: 851917
Summary: Move HTMLEmbedElement and HTMLAppledElement to WebIDL → Move HTMLEmbedElement and HTMLAppletElement to WebIDL
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.