Closed Bug 730318 Opened 8 years ago Closed 8 years ago

Opt-in activated plugins should use internal APIs to keep track of plugin activation

Categories

(Firefox for Android :: General, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 14
Tracking Status
blocking-fennec1.0 --- +

People

(Reporter: jaws, Assigned: jaws)

References

Details

Attachments

(2 files, 15 obsolete files)

9.89 KB, patch
Margaret
: review+
Details | Diff | Splinter Review
22.20 KB, patch
khuey
: review+
luke
: review+
Details | Diff | Splinter Review
Bug 719875 fixed an issue where plugins couldn't be activated when a page was loaded through the cache. This was fixed by setting a "played" attribute on the embed element.

We should rename this attribute to "mozActivated" so the connotation is clearer and the non-standardness is more explicit.

Desktop Firefox will also use the same attribute name, so consistency here is also important.
--- Comment #25 from Johnny Stenback (:jst, jst@mozilla.com) <jst@mozilla.org> 2012-02-24 08:34:04 PST ---
> Wait, our chrome code sets an attribute on the content DOM element here? That's
> generally not what we want, we don't want our chrome code injecting observable
> things into the DOM tree. Doing so causes problems with
> serialization/deserialization, cloning and using the cloned node, and can also
> generally confuse code on the page itself.

> If our UI code really needs to store the played state on a plugin in a content
> DOM then we should store that state through internal interfaces in ways where
> the state does not get copied when a node is cloned etc. We already have
> nsIObjectLoadingContent, can we move that state there?

I'll see how I can move this to internal APIs.
Summary: Opt-in activated plugins should use a Mozilla specific attribute name to keep track of plugin activation → Opt-in activated plugins should use internal APIs to keep track of plugin activation
If we fix bug 723617, we can probably just move back to storing the plugins in an array in browser chrome code, then we wouldn't need to worry about this.
Attached patch Patch for bug (obsolete) — Splinter Review
Margaret, can you test this out and make sure that this doesn't regress bug 719875?
Attachment #600600 - Flags: feedback?(margaret.leibovic)
Comment on attachment 600600 [details] [diff] [review]
Patch for bug

># HG changeset patch
># Parent 58dd942011a81f3149d9bc34e808806bda099056
># User Jared Wein <jwein@mozilla.com>
>Bug 730318 - Opt-in activated plugins should use internal APIs to keep track of plugin activation
>
>diff --git a/mobile/android/chrome/content/browser.js b/mobile/android/chrome/content/browser.js

>   playAllPlugins: function(aTab, aEvent) {

...

>+    for (let i in this._pluginsRequiringActivation) {

_pluginsRequiringActivation is stored on the tab, not PluginHelper, so you'll need to use aTab._pluginsRequiringActivation.

>+      let objLoadingContent = this._pluginsRequiringActivation[i].QueryInterface(Ci.nsIObjectLoadingContent);

If you're doing "for (let i in array)", i will be the plugin object, not an index, right?

I think you should probably just revert this code to the way we had it before, since that's least likely to cause accidental problems:

https://hg.mozilla.org/mozilla-central/file/8f2ca4c209e4/mobile/android/chrome/content/browser.js#l3732
Attachment #600600 - Flags: feedback?(margaret.leibovic) → feedback-
Also, is this bug just going to be about switching back the mobile front-end implementation? If so, we should re-summarize and move this to the Fennec Native component.
(In reply to Margaret Leibovic [:margaret] from comment #4)

> >+      let objLoadingContent = this._pluginsRequiringActivation[i].QueryInterface(Ci.nsIObjectLoadingContent);
> 
> If you're doing "for (let i in array)", i will be the plugin object, not an
> index, right?

Er, I got confused with for each. I just always use old-school for loops ;)
Yeah, this is only for Fennec Native. This doesn't exist yet in the desktop side of the browser, so that work will just be part of bug 711552.
Component: Plug-ins → General
Product: Core → Fennec Native
QA Contact: plugins → general
Attached patch Patch for bug v2 (obsolete) — Splinter Review
Thanks for catching the incorrect |this| in my previous patch. Can you take a look at this new version and see if it's what we want? Thanks!
Attachment #600600 - Attachment is obsolete: true
Attachment #601271 - Flags: feedback?(margaret.leibovic)
(In reply to Jared Wein [:jaws] from comment #8)
> Created attachment 601271 [details] [diff] [review]
> Patch for bug v2
> 
> Thanks for catching the incorrect |this| in my previous patch. Can you take
> a look at this new version and see if it's what we want? Thanks!

Hm, this patch looks like what we want, and it's working for me as it was before. However, I'm not getting PluginClickToPlay events on back/forward navigation (and plugins can't be activated), so it seems like bug 723617 isn't actually fixed...

I was testing navigating between http://people.mozilla.org/~mwargers/tests/plugins/flash/ and http://people.mozilla.org/~mwargers/tests/plugins/flash/flashembed.html.
Attached patch Patch for bug v3 (obsolete) — Splinter Review
+        // Add pageshow listener to the document to re-add the plugin to the
+        // array if it hasn't been enabled yet.
+        let doc = plugin.ownerDocument;
+        doc.addEventListener("pageshow", (function(event) {
+          let objLoadingContent = plugin.QueryInterface(Ci.nsIObjectLoadingContent);
+          if (!objLoadingContent.activated)
+            this._pluginsToPlay.push(plugin);
+        }).bind(this), true);
+

I'm not sure if these event listeners will need to be removed or not.

+    for (let plugin of plugins) {
+      let objLoadingContent = plugin.QueryInterface(Ci.nsIObjectLoadingContent);
+      objLoadingContent.playPlugin();
     }

I used the new for..of here. It is like "for each...in" but for arrays. https://developer.mozilla.org/en/JavaScript/Reference/Statements/for...of
Attachment #601271 - Attachment is obsolete: true
Attachment #601465 - Flags: feedback?(margaret.leibovic)
Attachment #601271 - Flags: feedback?(margaret.leibovic)
Comment on attachment 601465 [details] [diff] [review]
Patch for bug v3

(In reply to Jared Wein [:jaws] from comment #10)
> Created attachment 601465 [details] [diff] [review]
> Patch for bug v3
> 
> +        // Add pageshow listener to the document to re-add the plugin to the
> +        // array if it hasn't been enabled yet.
> +        let doc = plugin.ownerDocument;
> +        doc.addEventListener("pageshow", (function(event) {
> +          let objLoadingContent =
> plugin.QueryInterface(Ci.nsIObjectLoadingContent);
> +          if (!objLoadingContent.activated)
> +            this._pluginsToPlay.push(plugin);
> +        }).bind(this), true);
> +
> 
> I'm not sure if these event listeners will need to be removed or not.

Per IRC:
9:05 PM <Unfocused> jaws: yes, because callback function has a closure
Attachment #601465 - Flags: feedback?(margaret.leibovic)
blocking-fennec1.0: --- → ?
not blocking release.
blocking-fennec1.0: ? → -
Attached patch WIP patch for bug (untested) (obsolete) — Splinter Review
Kyle, I have tried to follow most of the steps that you have provided through IRC on how to go about building a list of plugins so that JS can read the list.

1) Is my current approach in the right direction?
2) I am currently using a nsTArray to store the plugins, which can be changed to a nsTHashtable without too much churn. The nsTHashtable will be faster, but for now I just want to get something working before I focus on optimization.
3) Is there a guaranteed one-to-one relationship between nsIObjectLoadingContent and nsIContent?
4) Can nsIObjectLoadingContent be QI'd to it's DOM element from script? It is my understanding that some DOM elements can be QI'd to nsIObjectLoadingContent, but I'm not sure about the other direction.
5) Can you help me with converting the nsTArray to either a nsIVariant or jsval for passing back to script? :bz mentioned that the function at http://mxr.mozilla.org/mozilla-central/source/dom/telephony/Telephony.cpp#73 will need to be fixed up to properly root the valArray and we'll also need to remove the dependencies on other Telephony.cpp functions such as ToISupports.
Attachment #601465 - Attachment is obsolete: true
Attachment #603041 - Flags: feedback?(khuey)
:bent fixed up nsTArrayToJSArray so the valArray would be properly rooted and with my local patch of bug 711552 I was able to test that the correct number of plugin objects is returned in the array.

However when I call a method on one of these objects in JS, I get a compartment mismatch. Can you help me narrow down why this is happening? I expect that it has to do with the nsTArrayToJSArray function or something else around passing native C++ objects to script.
Attachment #603041 - Attachment is obsolete: true
Attachment #603510 - Flags: feedback?(khuey)
Attachment #603041 - Flags: feedback?(khuey)
This patch has some changes to the nsTArrayToJSArray function so it is safer for callers to user and harder for them to use the wrong global object.

The compartment mismatch still happens, and I have filed bug 733636 for it.
Attachment #603510 - Attachment is obsolete: true
Attachment #603542 - Flags: feedback?(khuey)
Attachment #603510 - Flags: feedback?(khuey)
Attached patch Patch for bug (obsolete) — Splinter Review
Margaret: Can you test this out and see if it will work for mobile? I am able to use this patch as a base for my work on desktop.
Attachment #603542 - Attachment is obsolete: true
Attachment #603607 - Flags: feedback?(margaret.leibovic)
Attachment #603542 - Flags: feedback?(khuey)
Attachment #603607 - Attachment is obsolete: true
Attachment #603607 - Flags: feedback?(margaret.leibovic)
I tested this out on my HTC Sensation 4G and it works the same way as what we are shipping today. Deciding if the doorhanger will be shown is done during the |load| event so that there is more time for elements to resize.
Attachment #604025 - Flags: review?(margaret.leibovic)
Kyle: This is nearly done. The only thing that I am planning on changing is to convert the member of nsDocument from a nsTArray to a nsTHashtable, unless you see something else that should be done.

Can you take a look at this patch and see if there is anything else I should do before requesting review?
Attachment #604027 - Flags: feedback?(khuey)
(In reply to Jared Wein [:jaws] from comment #18)
> Created attachment 604025 [details] [diff] [review]
> Patch for bug (part 3): Update Fennec Native to use the new API for
> activating plugins

I used http://people.mozilla.org/~jwein/invisiblePlugin.html for testing "invisible" plugins.
I attached the wrong patch previously. This is the correct one.
Attachment #604027 - Attachment is obsolete: true
Attachment #604028 - Flags: feedback?(khuey)
Attachment #604027 - Flags: feedback?(khuey)
Removed a couple dump statements from the previous version.
Attachment #604025 - Attachment is obsolete: true
Attachment #604034 - Flags: review?(margaret.leibovic)
Attachment #604025 - Flags: review?(margaret.leibovic)
Comment on attachment 604034 [details] [diff] [review]
Patch for bug (part 3): Update Fennec Native to use the new API for activating plugins

Looks like we're on the right track here, but I found some issues to address:

>diff --git a/mobile/android/chrome/content/browser.js b/mobile/android/chrome/content/browser.js

>+      case "load": {
>+        // Show a plugin doorhanger if there are no clickable overlays showing
>+        let contentWindow = this.browser.contentWindow;
>+        let cwu = contentWindow.QueryInterface(Ci.nsIInterfaceRequestor)
>+                               .getInterface(Ci.nsIDOMWindowUtils);
>+        // XXX not sure if we should enable plugins for the parent documents...
>+        let plugins = cwu.plugins;
>+        for (let i in plugins) {

Weren't you going to use the new |for of|? I don't really care, just curious :)

>+          let plugin = plugins[i];
>+          let overlay = plugin.ownerDocument.getAnonymousElementByAttribute(plugin, "class", "mainBox");
>+          if (!overlay)
>+            continue;
>+          if (PluginHelper.isTooSmall(plugin, overlay)) {
>+            PluginHelper.showDoorHanger(this);

This will show a doorhanger as soon as we come across a plugin that's too small. Previously, we iterated through all the plugin elements, and only if there was no overlay showing would we show the doorhanger. I think we want to keep it that way for now (at least on mobile).

>+      case "pageshow": {
>+        // |this| needs to be the Tab object to get the reference to the function.
>+        window.removeEventListener("unload", this.onUnload, false);

I think you meant to get rid of this. You don't add this event listener anywhere.

>+        break;
>+      }
>       case "pagehide": {
>-        // Check to make sure it's top-level pagehide
>-        if (aEvent.target.defaultView == this.browser.contentWindow) {
>-          // Reset plugin state when we leave the page
>-          this._pluginCount = 0;
>-          this._pluginOverlayShowing = false;
>-        }
>         break;
>       }

Get rid of the pageshow and pagehide listeners now that they're not doing anything?

>-  playAllPlugins: function(aTab, aEvent) {
>-    if (aEvent) {
>-      if (!aEvent.isTrusted)
>-        return;
>-      aEvent.preventDefault();
>+  playAllPlugins: function(aContentWindow) {

Nice, I like the change to have this function take a window.
Attachment #604034 - Flags: review?(margaret.leibovic) → feedback+
(In reply to Margaret Leibovic [:margaret] from comment #24)

> >+          let plugin = plugins[i];
> >+          let overlay = plugin.ownerDocument.getAnonymousElementByAttribute(plugin, "class", "mainBox");
> >+          if (!overlay)
> >+            continue;
> >+          if (PluginHelper.isTooSmall(plugin, overlay)) {
> >+            PluginHelper.showDoorHanger(this);
> 
> This will show a doorhanger as soon as we come across a plugin that's too
> small. Previously, we iterated through all the plugin elements, and only if
> there was no overlay showing would we show the doorhanger. I think we want
> to keep it that way for now (at least on mobile).

Oh, I also just noticed that this won't show a doorhanger for plugins with no overlay (there can be hidden plugin elements on the page), so we definitely want to fix that.
Felipe and I were talking about the possibility of invisible plugins being added after the "load" event is fired and that there will be no doorhanger shown in this case. This issue exists with the current implementation of click-to-play on Fennec Native, so I did not try to fix it with this patch.

I will file a follow-up bug for that case.
Attachment #604034 - Attachment is obsolete: true
Attachment #604307 - Flags: review?(margaret.leibovic)
(In reply to Jared Wein [:jaws] from comment #26)

> Felipe and I were talking about the possibility of invisible plugins being
> added after the "load" event is fired and that there will be no doorhanger
> shown in this case. This issue exists with the current implementation of
> click-to-play on Fennec Native, so I did not try to fix it with this patch.
> 
> I will file a follow-up bug for that case.

If a plugin is dynamically added, shouldn't we be getting a PluginClickToPlay event? This is something that can be investigated in that follow-up, but perhaps we can figure out a way to show a doorhanger there in that case.
Comment on attachment 604307 [details] [diff] [review]
Patch for bug (part 2): Update Fennec Native to use the new API for activating plugins

This looks good. Thanks, Jared!
Attachment #604307 - Flags: review?(margaret.leibovic) → review+
I've switched nsDocument's plugin array to use an nsTHashtable now instead of an nsTArray.
Attachment #604028 - Attachment is obsolete: true
Attachment #604591 - Flags: review?(khuey)
Attachment #604028 - Flags: feedback?(khuey)
Attachment #604591 - Attachment description: 604028: Patch for bug (part 1): Implement a way for chrome js to enumerate the plugin objects on a page for activation. → Patch for bug (part 1): Implement a way for chrome js to enumerate the plugin objects on a page for activation.
Comment on attachment 604591 [details] [diff] [review]
Patch for bug (part 1): Implement a way for chrome js to enumerate the plugin objects on a page for activation.

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

Not bad, but I want to see it again.

::: content/base/public/nsIDocument.h
@@ +1565,5 @@
>    virtual nsresult SetImageLockingState(bool aLocked) = 0;
>  
> +  virtual nsresult AddPlugin(nsIObjectLoadingContent* aPlugin) = 0;
> +  virtual nsresult RemovePlugin(nsIObjectLoadingContent* aPlugin) = 0;
> +  virtual nsresult GetPlugins(nsTArray<nsIObjectLoadingContent*>& aPlugins) = 0;

Just stick this stuff on nsDocument directly and static_cast if you need to.

::: content/base/public/nsIObjectLoadingContent.idl
@@ +128,5 @@
> +  /**
> +   * This attribute will return true if the plugin has been activated
> +   * and false if the plugin is still in the click-to-play state.
> +   */
> +  readonly attribute boolean activated;

You need to change the UUID at the top of this file.

::: content/base/src/nsDocument.cpp
@@ +2036,5 @@
>    }
>  
> +  if (!mPlugins.Init()) {
> +    return NS_ERROR_OUT_OF_MEMORY;
> +  }

If (!mImageTracker.Init() ||
    !mPlugins.Init()) {
  return NS_ERROR_OUT_OF_MEMORY;
}

@@ +8282,5 @@
>  
> +nsresult
> +nsDocument::AddPlugin(nsIObjectLoadingContent* aPlugin)
> +{
> +  NS_ENSURE_ARG_POINTER(aPlugin);

Make this assert that aPlugin is not null (e.g. MOZ_ASSERT(aPlugin);)

@@ +8286,5 @@
> +  NS_ENSURE_ARG_POINTER(aPlugin);
> +  if (!mPlugins.PutEntry(aPlugin))
> +    return NS_ERROR_OUT_OF_MEMORY;
> +  return NS_OK;
> +}

Put this next to the other code you added.

@@ +8349,5 @@
>  
> +nsresult
> +nsDocument::RemovePlugin(nsIObjectLoadingContent* aPlugin)
> +{
> +  NS_ENSURE_ARG_POINTER(aPlugin);

Make this assert as well.  Once you do that, this function can never return anything but NS_OK, so you can make its return type void.

::: content/base/src/nsDocument.h
@@ +944,5 @@
>    virtual NS_HIDDEN_(nsresult) SetImageLockingState(bool aLocked);
>  
> +  virtual NS_HIDDEN_(nsresult) AddPlugin(nsIObjectLoadingContent* aPlugin);
> +  virtual NS_HIDDEN_(nsresult) RemovePlugin(nsIObjectLoadingContent* aPlugin);
> +  virtual NS_HIDDEN_(nsresult) GetPlugins(nsTArray<nsIObjectLoadingContent*>& aPlugins);

No need for virtual or NS_HIDDEN.

Brief comments on what these do would be nice too.

::: content/base/src/nsObjectLoadingContent.cpp
@@ +557,5 @@
> +
> +void
> +nsObjectLoadingContent::UnbindFromTree(bool /*aDeep*/, bool /*aNullParent*/)
> +{
> +  nsCOMPtr<nsIContent> thisContent = do_QueryInterface(static_cast<nsIObjectLoadingContent*>(this));

do_QueryObject(this);

@@ +577,5 @@
>    , mNetworkCreated(true)
>    // If plugins.click_to_play is false, plugins should always play
>    , mShouldPlay(!mozilla::Preferences::GetBool("plugins.click_to_play", false))
> +  // If plugins.click_to_play is true, track the activated state of plugins.
> +  , mActivated(!mozilla::Preferences::GetBool("plugins.click_to_play", false))

I don't think we want to grab a pref every time.  Check once at startup and cache the value somewhere?

::: content/base/src/nsObjectLoadingContent.h
@@ +247,5 @@
> +    virtual nsresult BindToTree(nsIDocument* aDocument, nsIContent* aParent,
> +                                nsIContent* aBindingParent,
> +                                bool aCompileEventHandler);
> +    virtual void UnbindFromTree(bool aDeep = true,
> +                                bool aNullParent = true);

No need for these to be virtual.

::: content/html/content/src/nsHTMLObjectElement.cpp
@@ +284,5 @@
>                                      bool aNullParent)
>  {
>    RemovedFromDocument();
>    nsGenericHTMLFormElement::UnbindFromTree(aDeep, aNullParent);
> +  nsObjectLoadingContent::UnbindFromTree(aDeep, aNullParent);

Unbind the object loading content first.

::: content/html/content/src/nsHTMLSharedObjectElement.cpp
@@ +303,5 @@
>                                            bool aNullParent)
>  {
>    RemovedFromDocument();
>    nsGenericHTMLElement::UnbindFromTree(aDeep, aNullParent);
> +  nsObjectLoadingContent::UnbindFromTree(aDeep, aNullParent);

Unbinding the object loading content should go first here.

::: dom/base/Makefile.in
@@ +163,5 @@
>  
>  include $(topsrcdir)/config/rules.mk
>  
>  LOCAL_INCLUDES += \
> +		-I$(topsrcdir)/js/xpconnect/public \

I would be surprised to find that this is actually necessary.

::: dom/interfaces/base/nsIDOMWindowUtils.idl
@@ +1053,5 @@
> +   * privileges.
> +   *
> +   */
> +  [implicit_jscontext]
> +  readonly attribute jsval plugins;

Again, need to change the uuid of the interface.

::: js/src/jsapi.cpp
@@ +4100,5 @@
>  JS_SetElement(JSContext *cx, JSObject *obj, uint32_t index, jsval *vp)
>  {
>      AssertNoGC(cx);
>      CHECK_REQUEST(cx);
> +    assertSameCompartment(cx, obj, *vp);

Why is that necessary?

::: js/xpconnect/public/nsTArrayHelpers.h
@@ +1,4 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +

This file needs include guards.
Attachment #604591 - Flags: review?(khuey) → review-
Comment on attachment 604026 [details] [diff] [review]
Patch for bug (part 2): Update dom/telephony to use new nsTArrayToJSArray implementation

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

I think the Telephony code has changed a bit here, you'll probably want to merge. r=me though.
Attachment #604026 - Flags: review?(bent.mozilla) → review+
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #30)
> Comment on attachment 604591 [details] [diff] [review]
> Patch for bug (part 1): Implement a way for chrome js to enumerate the
> plugin objects on a page for activation.
> 
> ::: content/base/public/nsIDocument.h
> @@ +1565,5 @@
> >    virtual nsresult SetImageLockingState(bool aLocked) = 0;
> >  
> > +  virtual nsresult AddPlugin(nsIObjectLoadingContent* aPlugin) = 0;
> > +  virtual nsresult RemovePlugin(nsIObjectLoadingContent* aPlugin) = 0;
> > +  virtual nsresult GetPlugins(nsTArray<nsIObjectLoadingContent*>& aPlugins) = 0;
> 
> Just stick this stuff on nsDocument directly and static_cast if you need to.

Removing these from the interface makes the implementation of nsDOMWindowUtils::GetPlugins much more complex. I would rather that we leave these on the interface.

> ::: content/base/src/nsObjectLoadingContent.cpp
> @@ +557,5 @@
> > +
> > +void
> > +nsObjectLoadingContent::UnbindFromTree(bool /*aDeep*/, bool /*aNullParent*/)
> > +{
> > +  nsCOMPtr<nsIContent> thisContent = do_QueryInterface(static_cast<nsIObjectLoadingContent*>(this));
> 
> do_QueryObject(this);

I tried making this change but I ran in to issues with "ambiguous access of 'QueryInterface' could be the 'QueryInterface' in base 'nsISupports' or could be the 'QueryInterface' in base 'nsISupports', etc. Could this come from multiple-inheritance? Either way, I think it would be simpler to leave it as-is. Also, without static_cast, this error occurs:
> error C2594: 'argument' : ambiguous conversions from 'nsObjectLoadingContent *const ' to 'nsISupports *'"
 
> @@ +577,5 @@
> >    , mNetworkCreated(true)
> >    // If plugins.click_to_play is false, plugins should always play
> >    , mShouldPlay(!mozilla::Preferences::GetBool("plugins.click_to_play", false))
> > +  // If plugins.click_to_play is true, track the activated state of plugins.
> > +  , mActivated(!mozilla::Preferences::GetBool("plugins.click_to_play", false))
> 
> I don't think we want to grab a pref every time.  Check once at startup and
> cache the value somewhere?

Good idea. I'm using a VarCache for this now.

> ::: dom/base/Makefile.in
> @@ +163,5 @@
> >  
> >  include $(topsrcdir)/config/rules.mk
> >  
> >  LOCAL_INCLUDES += \
> > +		-I$(topsrcdir)/js/xpconnect/public \
> 
> I would be surprised to find that this is actually necessary.

Without that line, this build error occurs:
> d:/mc/obj-dir/dom/base/../../../dom/base/nsDOMWindowUtils.cpp(80) : fatal error C1083: Cannot open include file: 'nsTArrayHelpers.h': No such file or directory
  
> ::: js/src/jsapi.cpp
> @@ +4100,5 @@
> >  JS_SetElement(JSContext *cx, JSObject *obj, uint32_t index, jsval *vp)
> >  {
> >      AssertNoGC(cx);
> >      CHECK_REQUEST(cx);
> > +    assertSameCompartment(cx, obj, *vp);
> 
> Why is that necessary?

The compartment mismatch that I ran into as part of bug 733636 could have been caught sooner if the assertSameCompartment also checked vp.
Attachment #604591 - Attachment is obsolete: true
Attachment #605664 - Flags: review?(khuey)
blocking a blocker
blocking-fennec1.0: - → +
(In reply to Brad Lassey [:blassey] from comment #33)
> blocking a blocker

to be clear this should legitimately block on its own merits. Having the click-to-play be hokey has caused me to enable flash always on my GN. That really isn't an option on pre-Honeycomb devices.
Blocks: 735833
Comment on attachment 605664 [details] [diff] [review]
Patch for bug (part 1 v2): Implement a way for chrome js to enumerate the plugin objects on a page for activation.

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

r-, but coming along nicely.  I expect the next review will be the last.

::: content/base/src/nsDocument.cpp
@@ +8363,5 @@
> +nsresult
> +nsDocument::AddPlugin(nsIObjectLoadingContent* aPlugin)
> +{
> +  MOZ_ASSERT(aPlugin);
> +  NS_ENSURE_ARG_POINTER(aPlugin);

If you assert, there's no need to NS_ENSURE_ARG_POINTER.

@@ +8365,5 @@
> +{
> +  MOZ_ASSERT(aPlugin);
> +  NS_ENSURE_ARG_POINTER(aPlugin);
> +  if (!mPlugins.PutEntry(aPlugin))
> +    return NS_ERROR_OUT_OF_MEMORY;

Brace your ifs.

@@ +8396,5 @@
> +}
> +
> +nsresult
> +nsDocument::GetPlugins(nsTArray<nsIObjectLoadingContent*>& aPlugins)
> +{

I think you should add

aPlugins.EnsureCapacity(aPlugins.Length() + mPlugins.Count());

which will ensure that we allocate the storage we need in one allocation up front instead of several times as we go.  This is pretty nitty for something that's not on a performance critical path ;-)

@@ +8399,5 @@
> +nsDocument::GetPlugins(nsTArray<nsIObjectLoadingContent*>& aPlugins)
> +{
> +  mPlugins.EnumerateEntries(AllPluginEnum, &aPlugins);
> +  EnumerateSubDocuments(AllSubDocumentPluginEnum, &aPlugins);
> +  return NS_OK;

If this always returns NS_OK, why have an nsresult return value?

::: content/base/src/nsObjectLoadingContent.cpp
@@ +121,5 @@
> +InitPrefCache()
> +{
> +  static bool initializedPrefCache = false;
> +  if (!initializedPrefCache)
> +    mozilla::Preferences::AddBoolVarCache(&gClickToPlayPlugins, "plugins.click_to_play");

Always brace your ifs in Gecko.

@@ +562,5 @@
> +                                   nsIContent* /*aBindingParent*/,
> +                                   bool /*aCompileEventHandlers*/)
> +{
> +  NS_ENSURE_ARG_POINTER(aDocument);
> +  return aDocument->AddPlugin(this);

aDocument can never be null, no need to check.

@@ +570,5 @@
> +nsObjectLoadingContent::UnbindFromTree(bool /*aDeep*/, bool /*aNullParent*/)
> +{
> +  nsCOMPtr<nsIContent> thisContent = do_QueryInterface(static_cast<nsIObjectLoadingContent*>(this));
> +  if (!thisContent)
> +    return;

Brace your ifs.

@@ +573,5 @@
> +  if (!thisContent)
> +    return;
> +  nsIDocument* ownerDoc = thisContent->OwnerDoc();
> +  if (!ownerDoc)
> +    return;

OwnerDoc() can never return null.  In Gecko GetFoo() returns a pointer that can be null, Foo() returns a pointer that cannot be null.

::: dom/base/Makefile.in
@@ +161,5 @@
>  
>  include $(topsrcdir)/config/rules.mk
>  
>  LOCAL_INCLUDES += \
> +		-I$(topsrcdir)/js/xpconnect/public \

Ok, nsTArrayHelpers should be added to EXPORTS in js/xpconnect/public/Makefile.in, and then this will be unnecessary.

::: dom/base/nsDOMWindowUtils.cpp
@@ +2181,5 @@
> +  }
> +
> +  nsresult rv;
> +  nsCOMPtr<nsIDOMDocument> ddoc;
> +  rv = mWindow->GetDocument(getter_AddRefs(ddoc));

nsIDOMDocument* ddoc = mWindow->GetExtantDocument();

@@ +2188,5 @@
> +  nsCOMPtr<nsIDocument> doc = do_QueryInterface(ddoc, &rv);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  nsTArray<nsIObjectLoadingContent*> plugins;
> +  doc->GetPlugins(plugins);

Why aren't you checking the return value here?

@@ +2194,5 @@
> +  JSObject* jsPlugins = nsnull;
> +  rv = nsTArrayToJSArray(cx, plugins, &jsPlugins);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  aPlugins->setObject(*jsPlugins);

*aPlugins = OBJECT_TO_JSVAL(jsPlugins);

::: js/src/jsapi.cpp
@@ +4106,5 @@
>  JS_SetElement(JSContext *cx, JSObject *obj, uint32_t index, jsval *vp)
>  {
>      AssertNoGC(cx);
>      CHECK_REQUEST(cx);
> +    assertSameCompartment(cx, obj, *vp);

A JS peer should sign off on this.
Attachment #605664 - Flags: review?(khuey) → review-
Luke: Please review the jsapi.cpp change (just updated the call to assertSameCompartment).
Attachment #605664 - Attachment is obsolete: true
Attachment #607348 - Flags: review?(luke)
Attachment #607348 - Flags: review?(khuey)
Attachment #607348 - Flags: review?(luke) → review+
Comment on attachment 607348 [details] [diff] [review]
Patch for bug (part 1 v3): Implement a way for chrome js to enumerate the plugin objects on a page for activation.

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

r=me with the comments addressed.

::: content/base/src/nsDocument.cpp
@@ +8381,5 @@
> +AllSubDocumentPluginEnum(nsIDocument* aDocument, void* userArg)
> +{
> +  nsTArray<nsIObjectLoadingContent*>* plugins =
> +    reinterpret_cast< nsTArray<nsIObjectLoadingContent*>* >(userArg);
> +  aDocument->GetPlugins(*plugins);

MOZ_ASSERT(plugins) here.

@@ +8390,5 @@
> +AllPluginEnum(nsPtrHashKey<nsIObjectLoadingContent>* aPlugin, void* userArg)
> +{
> +  nsTArray<nsIObjectLoadingContent*>* allPlugins =
> +    reinterpret_cast< nsTArray<nsIObjectLoadingContent*>* >(userArg);
> +  allPlugins->AppendElement(aPlugin->GetKey());

And MOZ_ASSERT(allPlugins).

::: content/base/src/nsObjectLoadingContent.cpp
@@ +571,5 @@
> +{
> +  nsCOMPtr<nsIContent> thisContent = do_QueryInterface(static_cast<nsIObjectLoadingContent*>(this));
> +  if (!thisContent) {
> +    return;
> +  }

This should never be null.  Please MOZ_ASSERT(thisContent).
Attachment #607348 - Flags: review?(khuey) → review+
And a much greener try run after I added a null check for aDocument in BindToTree:
https://tbpl.mozilla.org/?tree=Try&rev=1f7fe227e41d
Ah, yes, I should have caught that :-)
I forgot to add braces to the if condition on part 1 of the patch.
Attachment #608463 - Flags: review?(khuey)
https://hg.mozilla.org/mozilla-central/rev/62e3e0fc06c9 is breaking the b2g build:
/home/fabrice/dev/b2g/inbound/dom/telephony/Telephony.cpp: In member function 'virtual nsresult mozilla::dom::telephony::Telephony::GetCalls(jsval*)':
/home/fabrice/dev/b2g/inbound/dom/telephony/Telephony.cpp:309: error: 'mScriptContext' was not declared in this scope
Attachment #604307 - Attachment description: Patch for bug (part 3): Update Fennec Native to use the new API for activating plugins → Patch for bug (part 2): Update Fennec Native to use the new API for activating plugins
https://hg.mozilla.org/mozilla-central/rev/b60331eb2846
https://hg.mozilla.org/mozilla-central/rev/486ff4199ca3
Status: REOPENED → RESOLVED
Closed: 8 years ago8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.