Closed Bug 783059 Opened 12 years ago Closed 11 years ago

object/embed/applet tags need instantiation tests

Categories

(Core Graveyard :: Plug-ins, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla22

People

(Reporter: johns, Assigned: johns)

References

Details

Attachments

(2 files, 5 obsolete files)

In cases like bug 782707 and bug 782703 it's clear that we don't have good test coverage for making sure objects instantiate when they should. This is mostly because the pre-bug 745030 logic for "when they should" was approximately "sometimes, maybe"

Incomplete list of things we need coverage on
- Removing and re-adding to the document
- obj.data = obj.data while in and out of the document
- Changing obj.data multiple times with and w/o an associated channel
- Changing object type hints without changing channel
- Moving into subdocuments that change the effective URI, when obj.data is relative
also,
- Move running plugins between documents, make sure they don't reinstantiate, but do if left out for a full event loop cycle. Try changing plugin parameters while in this limbo state.
Depends on: 767631
Blocks: 767631
No longer depends on: 767631
Blocks: 767627
Blocks: 738396
Depends on: 812629
No longer blocks: 767627
Depends on: 784131
Depends on: 832032
Attached patch Plugin instantation test matrix (obsolete) — Splinter Review
Still need to make hasRunningPlugin callable via SpecialPowers to test for instantiation....
@bz

This might be useful for testing the object/embed webidl stuff, though it does not much touch script access. It also takes a good thirty seconds to run.
30 seconds is nothing compared to the time I've already spent on this junk.  Running pretty much any plug-in test in our test harness takes that long.

I'll run this in my local build with all those changes.
Hmm.  Do we want to land this first and then the WebIDL stuff?  Or the other way around? The merge is not quite trivial: hasRunningPlugin needs a WebIDL version.  Just trying to figure out which direction I should merge in locally.
I still need to fix the hasRunningPlugin stuff to actually work right, so I wouldn't block the webidl changes on this.
Attached patch Plugin instantation test matrix (obsolete) — Splinter Review
@bz this version has finished hasRunningPlugin checks and tries to poke at the plugin to test scriptability in all cases where it finds it should be instantiated.

It also is up to 3 minutes to run, which I have to work on...
Attachment #717416 - Attachment is obsolete: true
Well, the whole 14000-test thing probably doesn't help.  ;)

I can't manage to run this test: it always takes longer than the test harness timeout for me (in a debug build, granted).
Attached patch Plugin instantiation test matrix (obsolete) — Splinter Review
This should take an order of magnitude less time
Attachment #718189 - Attachment is obsolete: true
Ah, great.  That runs and passes with my WebIDL patches.
Attachment #718634 - Attachment is obsolete: true
Attachment #722572 - Flags: review?(joshmoz)
Comment on attachment 722573 [details] [diff] [review]
Add do_lookupGetter helper to SpecialPowers

The much simpler than we discussed but still groan-inducing helper needed for the test.
Attachment #722573 - Flags: review?(bobbyholley+bmo)
Comment on attachment 722573 [details] [diff] [review]
Add do_lookupGetter helper to SpecialPowers

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

Please add a comment explaining why this is necessary. r=bholley with that.

::: testing/specialpowers/content/specialpowersAPI.js
@@ +710,5 @@
>       obj2=unwrapIfWrapped(obj2);
>       return obj1 instanceof obj2;
>    },
>  
> +  do_lookupGetter: function(obj, getter) {

I'd call the param "name" rather than "getter".
Attachment #722573 - Flags: review?(bobbyholley+bmo) → review+
Depends on: 786650
Depends on: 803159
Comment on attachment 722572 [details] [diff] [review]
Plugin instantiation tests for embed/object tags

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

::: content/base/src/nsObjectLoadingContent.h
@@ +199,5 @@
>      }
> +    bool HasRunningPlugin() const
> +    {
> +      return !!mInstanceOwner;
> +    }

Where is this used?
Attachment #722572 - Flags: review?(joshmoz) → review+
(In reply to Josh Aas (Mozilla Corporation) from comment #15)
> Comment on attachment 722572 [details] [diff] [review]
> Plugin instantiation tests for embed/object tags
> 
> Review of attachment 722572 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/base/src/nsObjectLoadingContent.h
> @@ +199,5 @@
> >      }
> > +    bool HasRunningPlugin() const
> > +    {
> > +      return !!mInstanceOwner;
> > +    }
> 
> Where is this used?

This is the webIDL equivalent to GetHasRunningPlugin() for the XPIDL, though GetHasRunningPlugin should just be calling this!

The test uses this to test if the plugin spawned of its own accord (if we just call .getObjectValue() it will force-spawn the plugin if it hasn't yet)
Comment on attachment 722573 [details] [diff] [review]
Add do_lookupGetter helper to SpecialPowers

https://hg.mozilla.org/integration/mozilla-inbound/rev/36cde68b1bf4
Attachment #722573 - Flags: checkin+
Comment on attachment 722572 [details] [diff] [review]
Plugin instantiation tests for embed/object tags

https://hg.mozilla.org/integration/mozilla-inbound/rev/9f5cff5a1a3d
Attachment #722572 - Flags: checkin+
Comment on attachment 722572 [details] [diff] [review]
Plugin instantiation tests for embed/object tags

Blew up on android.
https://hg.mozilla.org/integration/mozilla-inbound/rev/ad90d1366dba
Attachment #722572 - Flags: checkin+ → checkin-
Attachment #722573 - Flags: checkin+ → checkin-
Exclude this test on android since we have no test plugin...
Attachment #722572 - Attachment is obsolete: true
Comment on attachment 722986 [details] [diff] [review]
Plugin instantiation tests for embed/object tags. r=josh

Attempt #2

https://hg.mozilla.org/integration/mozilla-inbound/rev/18df15462240
Attachment #722986 - Flags: checkin+
Comment on attachment 722988 [details] [diff] [review]
Add do_lookupGetter helper to SpecialPowers. r=bholley

https://hg.mozilla.org/integration/mozilla-inbound/rev/c7ff3b1f040f
Attachment #722988 - Flags: checkin+
https://hg.mozilla.org/mozilla-central/rev/c7ff3b1f040f
https://hg.mozilla.org/mozilla-central/rev/18df15462240
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: