Flash crashed while playing a video in full-screen mode. The crashed-plugin UI was correctly shown, filling the screen. But seems like it should be polite and exit FS mode when this happens. (I was able to get out of it by hitting ESC.)
Is fixing this as simple as checking document.mozFullScreenElement == self.plugin plus a document.mozCancelFullScreen()? Not sure if plugins do something special for FS.

It was on Vimeo (eg http://vimeo.com/74324610), which seems to be using HTML fullscreen (for example, I get a Firefox right-click context menu, and the fullscreen-notice message appears to be Firefox's). Although a quick poke with Inspector seems to indicate the video is still Flash... Are the perhaps doing a hybrid?
Definitely a different experience than what YouTube does.

That's actually really great news (I've been asking both Vimeo and Youtube to do this). And that makes this fairly easy to fix.
Within http://hg.mozilla.org/mozilla-central/annotate/bdac18bd6c74/browser/base/content/browser-plugins.js#l1143 we should check whether there is a fullscreen element, and if the plugin is or is within the fullscreen element we should exit fullscreen mode.
A testcase needs to be added similar to dom/plugins/test/mochitest/test_crashing.html which covers this case and checks after the crash to make sure that fullscreen was cancelled and the page was notified correctly.

Sushrut, that's great. We don't change the bug assignee until you've written and attached the first patch. Please let me know if anything is unclear about the bug or you need help. I suggest you start by creating a minimal testcase.

Comment on attachment 8485458[details][diff][review]
A proposed patch for the Flash full screen issue
Review of attachment 8485458[details][diff][review]:
-----------------------------------------------------------------
As pointed out by prior comments, we should at least check
* if we are in |document.mozFullScreen| and
* if the |plugin| is |document.mozFullScreenElement| or a child of it
If a plugin crashes that's not even part of the fullscreen elements, it probably doesn't matter to the user experience.
Let me know if you need any help with this Amam.

Comment on attachment 8553612[details][diff][review]
bud-1028200-fix.patch
Thank you for the patch. It's not quite correct yet, but it's a good start and just needs some additional checks:
This will cancel full-screen even if the plugin is not within the full-screen element. That doesn't sound like an obviously good idea. Instead of doing this unconditionally, can you also check whether the plugin element is within the full-screen element?
Also this deserves a test; I put some details about that in comment 3, but let me know if you need more guidance about how to write a test for this.

Comment on attachment 8555371[details][diff][review]
bug-1028200-fix-v2.patch
The full-screen element will almost never be a <video> element. For example for youtube is a <div> in which is nested various other bits incluging the controls and the <video> element itself.
But also it seems that you're doing this in the parent process, and specifically on a UI trigger. Try doing it here instead:
http://hg.mozilla.org/mozilla-central/annotate/1dd013ece082/browser/modules/PluginContent.jsm#l855
And then you can check the parent chain of the crashed plugin, looking for the full-screen element like so:
let fullScreenElement = document.mozFullScreenElement;
if (fullScreenElement) {
let node = target;
while (node) {
if (fullScreenElement === node) {
document.mozCancelFullScreen();
}
node = node.parentElement;
}
}

Comment on attachment 8556400[details][diff][review]
bug-1028200-fix.patch
This looks good; you need to fix the indentation of the line "let fullScreenElement".
You also need to have a test for this. I can't mark r+ without a test.

How are you supposed to link `PluginContent.jsm` to the test (or otherwise get it to run)?
Using `test_crashing.html` as a template, the code in PluginContent.jsm doesn't fire when method `p.crash()` is called. Looking at the inspector shows that PluginContent.jsm isn't even loaded.
I've tried symlinking to any relevant looking directories and then adding it with `<script src=...>` in the test html file but the file is never found. [This link](https://developer.mozilla.org/en-US/docs/Mozilla/JavaScript_code_modules/Using) is supposedly using deprecated code (I get a warning about Component being deprecated). And another method (of which I can't remember right now) returned a path where it is also never found.

Thanks, that seemed like what I was looking for.
I'm still having trouble with seeing `PluginContent.jsm` actually do anything though.
I tried putting breakpoints all around the file but couldn't get any of them to trip at all.
In particular, I was expecting `p.crash()` to trigger `PluginContent.onPluginCrashed()`.
What am I missing?
I was running mochitest with test_crashing.html on changeset:308747 for reference.

Is there a way to get any form of feedback from PluginContent.jsm?
Breakpoints don't work and, console.log() and alert() are undefined when testing with Firefox's Browser Toolbox with options ticked: "Enable browser chrome and add-on debugging toolboxes" and "Enable remote debugging".

I'm surprised, though I don't know why. Some people on IRC suggested closing and re-opening your toolbox might help. Beyond that I'm afraid I can't be a very good mentor: I suggest asking on the #e10s channel in IRC.

Created attachment 8784664[details][diff][review]
patch_base=changeset:308747
1. Because the test harness uses iframes, the `else branch` for `PluginContent.jsm` will never actually be traversed. This has to do with the fact that content within an iframe is hidden from `mozFullScreenElement` (it only shows the iframe itself as fullscreen).
You can test the other path by running
./mach mochitest dom/plugins/test/mochitest/test_bug1028200.html --jsdebugger
in the top level directory and then manually clicking on the particular test to run it in its own page.
2. Also, rarely the test will fail to crash the plugin. It'll go fullscreen and that's it. A refresh will fix that but it's still annoying. I don't know if it has anything to do with my solution in particular.
I'm not sure what to do about these issues.

Comment on attachment 8785646[details][diff][review]
patch_base=changeset:308747_rolled
Feedback so far:
* please use spaces and not hard tabs. I believe that this file is eslint-checked, so you should be able to run our linter to check basic style rules
* there's a bunch of other whitespace which is inconsistent with our normal coding guide:
** (arguments) shouldn't have extra spaces
** ) { space before braces
** if ( space after if/else control statements
** See https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style for our style guide
Algorithmically, I'm pretty sure we don't need a search algorithm for this. Traversing the entire DOM tree looking for a particular element would be pretty terrible for performance/memory usage. Without iframes, you should be able to just loop over .parentElement to find the fullscreen element (or not). With iframes it's a little trickier, but with chrome privileges it should be possible to navigate "upwards" using window.frameElement even across origins.

Created attachment 8787063[details][diff][review]
Style + Algorithm Change
I'm not sure if I'm supposed to submit this as an all-inclusive patch or not. I thought it would be easier to see the changes made to it as a continuation of the previous patch so I did it this way.
1. Style Changes made.
2. Replaced iframe search algorithm.
Regarding PluginContent.jsm:
window.frameElement always refers to the top level document so it's always null (as far as I can tell anyways, I haven't looked into getting normal feedback from PluginContent.jsm yet, but using the browser toolbox I get null). It turns out though that mozFullScreenElement works within the iframe element to get the actual fullscreen element. So I used that instead.

Comment on attachment 8787826[details][diff][review]
Style + Algorithm Change
I'm sorry I didn't get to this Friday. In terms of reviewing, it would be better to attach a single rollup patch that includes all the changes in one patch.
Does the test cover all of the cases you care about? In particular:
* a toplevel window with a plugin in a fullscreen element
* a window with a same-origin iframe, fullscreen element in the iframe, and plugin within that
* a window with a fullscreen element, containing a same-origin iframe, containing a plugin
* a window with a cross-origin iframe, fullscreen element in the iframe, and plugin within that
* a window with a fullscreen element, containing a cross-origin iframe, containing a plugin

Can you clarify the difference between cross-origin and same-origin iframe? I'm not sure what that means.
My first thought was that same-origin means something like:
<iframe>
<p> same origin element </p>
</iframe>
where as cross-origin would mean:
<iframe src="cross_origin_source.html"></iframe>
But I don't think iframe works without the src attribute; at least I can't get content in the first example to show.

Feel free to use NEEDINFO when you ask questions, so that I see them faster.
Same "same origin" I mean coming from the same website.
So e.g. this is a same-origin iframe:
http://foo.com/x.html has an <iframe src="http://foo.com/y.html" allowfullscreen>
And this is a cross-origin iframe:
http://foo.com/x.html has an <iframe src="http://bar.com/y.html" allowfullscreen>
The reason this makes a difference is that content script can navigate the DOM of a same-origin iframe.contentDocument. But only chrome script can access the DOM across multiple origins.

Firstly, I've been trying to implement your solution (strictly upward traversal from crashed plugin to fullscreen element) and "frameElement" is always "null".
I've figured out how to "console.log":
this.global.console.log();
I've since been checking as many attributes as I could starting from "this.global.* ", "this.content.* " and even looked up "this.*" but every time "frameElement" was always null and any other attributes I thought might been of relevance didn't hold anything useful.
Second, I also tried grepping all "jsm" files for frameElement but haven't been able to trace it back to some module or library which might provide some leads.
Lastly, I did notice that the codebase contains a file "/addon-sdk/source/lib/sdk/window/utils.js" that seems to contains a special "getFrameElement" function that may or may not help. I can't import it though since PluginContent is a jsm file.
So I'm stuck :(
I don't know how to proceed from here.
The reason I'm trying to implement this solution is because my current solution can't cover all of the other cases.

I think we might be able to simplify this a bit.
First of all, none of this is required if we're not in fullscreen mode, and we can check that by checking to see whether or not document.fullscreenElement is non-null.
If it's non-null, we have a few cases to consider:
1) The crashed plugin is contained within the fullscreen element, not crossing an iframe boundary
2) The crashed plugin is contained within the fullscreen element, but across one or more iframe boundaries
3) The crashed plugin is not contained within the fullscreen element
Testing (1) is made simple by Node.contains: https://developer.mozilla.org/en-US/docs/Web/API/Node/contains
Testing (3) is made simple by testing (2), since if both (1) and (2) are false, (3) must be true.
So that leaves testing (2).
With privileged JS, I think we should be able to do quick hops up to containing scopes, like this:
```Pseudocode-ish JS
// Assume "plugin" is our crashed plugin DOM node
if (document.fullscreenElement) {
if (containsNodeAcrossIframes({ container: document.fullscreenElement, candidate: plugin }) {
// The plugin was inside a fullscreen element!
}
}
// ...
function containsNodeAcrossIframes({ container, candidate }) {
if (container.contains(candidate)) {
return true;
}
if (candidate.ownerGlobal.frameElement) {
// The candidate is inside an iframe! Grab the frameElement, which is the <iframe>
// in the containing document, and then recursively check to see if that's held
// within container.
return containsNodeAcrossIframes({ container, candidate: candidate.ownerGlobal.frameElement });
}
// The candidate wasn't inside the container, and the candidate also wasn't inside
// an iframe. No other possibilities.
return false;
}
```
Does any of the above help?

Created attachment 8796546[details][diff][review]bug1028200_2.diff
I decided not to make tests for cross-origin iframes because they seem to
behave the same as same-origin iframes.
I made these test based on taking the permuation of three things: div, iframe
and plugin. Then I removed some that I felt were the same as one of the
following tests:
1. *div -> iframe -> plugin - exercises the upward traversal algorithm
2. plugin ; *div - most basic case with div for not contains
3. iframe -> *div -> plugin - exercises the downward traversal algorithm
4. *iframe -> plugin - most basic case with iframe
5. *div -> plugin - most basic case with div
6. plugin ; *iframe - most basic case with iframe for not contains
7. iframe -> plugin ; iframe -> *div - most complicated case using both traversals
and also finding the plugin is not contained
in the fullscreen element
* - the fullscreen element
I actually doubled up on the iframes so its actually `div -> iframe -> iframe
-> plugin` for example. This is just because I noticed that it might be a
problem with the solution I had at the time.

Comment on attachment 8796546[details][diff][review]bug1028200_2.diff
There are some small formatting nits, but this is basically ready for review. Please fix the following issues and then request review from mconley:
* hard tabs. This patch is littered with hard tabs, which are almost never allowed in our sources. Please find these and replace with spaces. Also I'll suggest configuring your editor/IDE to use spaces instead of tabs by default. This is true in both JS and HTML (the tests).
We prefer that all "if" statements use multiple lines and braces, so e.g.
+ if (fullScreenElement.tagName === "IFRAME")
+ fullScreenElement = traverseDownwardToTrueFullScreenElement(fullScreenElement);
should have braces, and
+ if (fullScreenElement.contains(crashedPlugin)) return true;
should have braces and multiple lines
I'm slightly worried about the cross-origin case, but if you've tested it manually and are confident that your code works correctly, I suppose we could avoid automated testing for that case.

I'm having issues with eventListener and SpawnTask:
I have a test case which expects the full screen element to remain. When I flip the expectation to incorrectly expect that the full screen element be cancelled, it still passes. This is because the eventListener will pass once the full screen is cancelled, and I can't see any results until I manually cancel the full screen. I'm not sure how to proceed other than to have a timer on it (which is the current implementation).
I'm not totally sure what SpawnTasks does from what I quickly read, but I did read that it will automatically call SimpleTest.finish() once it is done. I have need to manually call SimpleTest.finish() since I want to manually call mozCancelFullScreen once SimpleTest.finish() has been called. Is it still a good idea to use SpawnTask?

Created attachment 8805021[details][diff][review]bug1028200_3.diff
1. eslint gives the following warning:
{{{
1:1 warning Definition for rule 'mozilla/import-globals' was not found mozilla/import-globals
}}}
is this something to be worried about?
2. "My above comment will cause us to traverse down to the lowest fullscreen element that might contain the plugin (or null if things go wrong). We can probably just do the .contains check on that last element and be done, or am I missing a case?"
* a potential case was with something like:
<iframe(s)>
<fullscreen div>
<iframe(s)>
<plugin>
So the algorithm would go down to the div and then stop - meanwhile the plugin is contained within the iframe(s) below the div.
Tests 1 and 7 are good examples.

Created attachment 8808906[details][diff][review]bug1028200_4.diff
Generalized as much as I could so I could put it into the general `head.js`
file to avoid the `1028200-head.js` file. That would've meant that there
would've be a lot of repeated code specific to my tests only (but
shared among my tests). Not sure if this is what you meant. I ran into an
issue however:
There is something mystical about `head.js`:
1. `head.js` is reference-able despite not being included in `mochitest.ini`
2. *Exactly one of my tests fail (5)* when using `head.js` but *does not fail*
when using the same functions referenced from `1028200-head.js`. Half of my
tests (1,3,4) use exactly the same code (from either heads) but do not fail.
3. That same test (5) *does not fail when run as a group*, but
*does fail when run individually*
(`./mach mochitest dom/plugins/test/mochitest/test_bug1028200` vs
`./mach mochitest dom/plugins/test/mochitest/test_bug1028200-5`)
4. There is one reference of `head.js` in `browser.ini` but removing it doesn't
change anything
Maybe you have information about this?
For now, I just put everything back into `1028200-head.js`. This also helps to
cut down on the repeated code among my tests.
By the way can you assign the bug to me? I hadn't noticed this until now but
the bug is not assigned.

Comment on attachment 8808906[details][diff][review]bug1028200_4.diff
Review of attachment 8808906[details][diff][review]:
-----------------------------------------------------------------
Hey s7hoang,
Thanks for the patch! The fix looks good - I just have some feedback for the tests.
So the idea behind "head.js" is that all tests within the same directory will share the functions in head.js - head.js will be evaluated in the scope of the test, which makes it a place to put utility functions that might be shared across multiple tests.
Putting test-specific things inside head.js (like the boilerplate inside 1028200-head.js, for example) should not go inside a head.js.
Instead, I recommend that you only put utility functions inside head.js. Those utility functions should probably return Promises if they do things like "waiting for".
So here are the concrete things that I suggest you do to get into a landable state:
1) Make all of the utility functions you defined in 1028200-head.js return Promises.
2) Move those utility functions into head.js
3) Drop the usage of "SimpleTest.waitForExplicitFinish();" or "SimpleTest.finish()", and use SpawnTask instead, which will make it easier to work with the Promises.
4) Move any of the simple remaining boilerplate into the tests themselves.
Let me know if you have any questions! Thanks for your patience and persistence with this one!
::: browser/modules/PluginContent.jsm
@@ +939,5 @@
> * fired for both NPAPI and Gecko Media plugins. In the latter case, the
> * target of the event is the document that the GMP is being used in.
> */
> onPluginCrashed: function (target, aEvent) {
> +
Don't need the newline here.
::: dom/plugins/test/mochitest/1028200-head.js
@@ +10,5 @@
> +
> +SimpleTest.registerCleanupFunction(() => {
> + this.document.mozCancelFullScreen();
> +});
> +/* --- /boilerplate --- */
The "boilerplate" comments don't add too much, I don't think.
@@ +13,5 @@
> +});
> +/* --- /boilerplate --- */
> +
> +/**
> + * Creates an event listener which removes itself upon activating so that it
Should probably specific that this is the fullscreenchange event we're listening for.
@@ +16,5 @@
> +/**
> + * Creates an event listener which removes itself upon activating so that it
> + * only triggers once.
> + * @param callback User supplied function to execute upon activating the event
> + * listener */
Nit - comment block closers should be on their own line.
@@ +17,5 @@
> + * Creates an event listener which removes itself upon activating so that it
> + * only triggers once.
> + * @param callback User supplied function to execute upon activating the event
> + * listener */
> +function addOneTimeFullScreenChangeEventListener(callback) {
This is pretty verbose. How about:
waitForFullScreenChange ?
(And even better, have it return a Promise, and use SpawnTask, since that'll make these functions much easier to read and reason about).
@@ +34,5 @@
> + * @param success Callback which is executed upon successfully crashing the plugin.
> + * @param fail Callback which is executed upon unsuccessfully crashing the plugin.
> + * @param after Callback which is executed after after attempting to crash
> + * the plugin (regarless of whether it was successful or not) */
> +function crashPlugin(plugin, success, fail, after) {
I think I'd prefer if this returned a Promise that resolve's if the crash proceeds, and rejects if it doesn't.
@@ +55,5 @@
> + * fullscreen element. Executed regardless of whether the check
> + * was successful or not because of how `waitForCondition` works.
> + * @param errMsg The message to show if it fails to find no fullscreen
> + * element. Equivalent to `ok(false, errMsg)` */
> +function checkCancelledFullScreen(callback, errMsg) {
Is this even necessary? If we use SpawnTask and waitForFullScreenChange, the users of this function could set up a Promise, like this:
let changePromise = waitForFullScreenChange();
yield crashPlugin(plugin);
yield changePromise;
// At this point, we know that we've exited fullscreen, because fullscreenchange fired.
@@ +72,5 @@
> + * was successful or not because of how `waitForCondition` works.
> + * @param errMsg The message to show if it fails to find a fullscreen
> + * element. Equivalent to `ok(false, errMsg)` */
> +function checkStillFullScreen(callback, errMsg) {
> + SimpleTest.waitForCondition(() => {
What we might be able to do instead is return a Promise that resolves after 5s or so, but rejects (and cancels its timeout) if it sees the fullscreenchange event.
@@ +82,5 @@
> + * Function to test that a fullscreen element gets cancelled after the plugin
> + * crashes.
> + * @param fullScreenElement The target fullscreen element
> + * @param plugin The target crashing plugin */
> +function testFullScreenCancelled(fullScreenElement, plugin) {
With SpawnTask, I actually think we can go ahead and just have this exist in your .html test files as:
add_task(function*() {
// ... get fullScreenElement and plugin references
let fullscreen = waitForFullScreenChange();
fullScreenElement.mozRequestFullScreen();
yield fullscreen;
// We're fullscreen now. Set up the next fullscreen change
// listener
let fullscreenExit = waitForFullScreenChange();
yield crashPlugin(plugin);
yield fullscreenExit;
ok(!document.mozFullScreenElement, "Element is not fullscreen anymore");
});
Same applies down below, but using the timeout function I suggest above.
::: dom/plugins/test/mochitest/test_bug1028200-1.html
@@ +1,3 @@
> +<head>
> + <meta charset="utf-8">
> + <title>Plugin Crash, FullScreenElement Cancelled, div -> iframe -> iframe -> plugin</title>
Can you also indicate which element is fullscreened in this title?

Created attachment 8812542[details][diff][review]bug1028200_5.diff
So, test 5 still fails the first time but doesn't afterwards. I don't know why but I don't think it has to do with my code since it works just fine when using the same functions in a file other than "head.js".

Created attachment 8814285[details][diff][review]bug1028200_6.diff
"Why is this check necessary with polling? Shouldn't the mozFullScreenElement be cleared after fullscreenchange is fired?"
There is one reason and two applications:
The reason is that the second mozFullScreenChange is not guaranteed to occur.
The applications are:
1. tests which are expecting the absence of the second `mozFullScreenChange`
(tests# 2,6,7). In this case, the second `mozFullScreenChange` should
indicate failure.
2. sanity checking tests which are expecting the occurrence of the second
`mozFullScreenChange` (any test that say "FullScreen Cancelled"). In this
case, the test will not fail if intentionally swapped with a "FullScreen
Remains" section. Instead, the fullscreen element will sit there until
manually exited because the second `mozFullScreenChange` never occurs.
However, manually triggering the exit will then cause the test to actually
pass instead of failing because `mozFullScreenChange` will trigger.
In total there are 4 situations we are accounting for:
element exits fullscreen , expect element to exit fullscreen
(tests #1,3,4,5)
element exits fullscreen , expect element to remain fullscreen
(intentionally swapping sections on tests# 1,3,4,5 with tests# 2,6,7)
element remains fullscreen, expect element to exit fullscreen
(intentionally swapping sections on tests# 2,6,7 with tests# 1,3,4,5)
element remains fullscreen, expect element to remain fullscreen
(tests #2,6,7)
I think I've gotten the hang of promises now and can better realize what I was
trying to achieve. On simplifying the code, I found that the second event
listener is unnecessary when done with polling. It is slow to execute though
because we're not manually calling `SimpleTest.finish()` anymore (in
conjunction with eventlistening) so we're waiting out the timer each test.

Comment on attachment 8814285[details][diff][review]bug1028200_6.diff
Review of attachment 8814285[details][diff][review]:
-----------------------------------------------------------------
Hey! Thanks - this is shaping up nicely. A few comments - see below.
> On simplifying the code, I found that the second event
> listener is unnecessary when done with polling. It is slow to execute though
> because we're not manually calling `SimpleTest.finish()` anymore (in
> conjunction with eventlistening) so we're waiting out the timer each test.
In the cases where we want to ensure that the event _didn't_ fire, we'll want to wait it out. In the cases where we want the event to fire, and it does, we can cancel the timer (see my comment in the first test about the timer for context), and resolve the Promise immediately and move on.
::: dom/plugins/test/mochitest/head.js
@@ -130,4 @@
> ok(false, "Unable to find plugin");
> return null;
> }
> -
Please revert this change - we should keep the newline at the end of the file.
::: dom/plugins/test/mochitest/plugin-utils.js
@@ +102,5 @@
> + * Returns a promise which resolves on `mozFullScreenChange`.
> + */
> +function promiseFullScreenChange(){
> + return new Promise(resolve => {
> + document.addEventListener("fullscreenchange", resolve);
We need to remember to remove the fullscreenchange event listener after it has fired. Somethinglike:
return new Promise(resolve => {
document.addEventListener("fullscreenchange", function onFullScreen(e) {
document.removeEventListener("fullscreenchange", onFullScreen);
resolve();
});
});
but also with that timeout feature I mentioned earlier (don't forget to have it clear the timeout if the event is seen).
::: dom/plugins/test/mochitest/test_bug1028200-1.html
@@ +57,5 @@
> + they can fail and that they fail gracefully (vs event listening).
> + This is done by intentionally expecting the wrong outcome (not to be
> + confused with "not passing the condition" which this test is
> + actually expecting). This is also the reason why this task is its
> + own task and not integrated with the above task (visual separation).
I appreciate the time you put into putting this comment together and laying out your reasoning!
I see where you're coming from. We're in a situation where we expect that, eventually, the fullscreen element will be cleared, and we want to properly detect the failure case.
I think I have a suggestion that's more idiomatic:
1) Augment promiseFullScreenChange to reject after a 5s timeout
2) Use promiseFullScreenChange to wait for the event (or the timeout)
3) Once the promiseFullScreenChange resolves, do a sanity check on document.mozFullScreenElement being null
I think I'd much prefer that over polling the DOM, to be honest - and it's more in line with examples I've seen elsewhere in our codebase.
::: dom/plugins/test/mochitest/test_bug1028200-2.html
@@ +47,5 @@
> + });
> +
> + add_task(function* () {
> + /*
> + Poll for the fullscreen element still being fullscreen. After
For this one, I think what we can do is use the promiseFullScreenChange augmentation I suggested earlier, and use that with your "fail in then, pass in catch" structure. Also not a bad idea to ensure that document.mozFullScreenElement is still set.
@@ +74,5 @@
> + ok(false, "Element is still fullscreen");
> + })
> + .catch(() => {
> + ok(true, "Element is still fullscreen");
> + });
We should make sure to exit full screen after this test.
::: dom/plugins/test/mochitest/test_bug1028200-3.html
@@ +62,5 @@
> +
> + =)
> + */
> +
> + yield SimpleTest.promiseWaitForConditionWithReject(() => {
I think same approach as test_bug1028200-1.html will work here.
::: dom/plugins/test/mochitest/test_bug1028200-4.html
@@ +62,5 @@
> +
> + =)
> + */
> +
> + yield SimpleTest.promiseWaitForConditionWithReject(() => {
I think same approach as test_bug1028200-1.html will work here.
::: dom/plugins/test/mochitest/test_bug1028200-5.html
@@ +59,5 @@
> +
> + =)
> + */
> +
> + yield SimpleTest.promiseWaitForConditionWithReject(() => {
As above.
::: dom/plugins/test/mochitest/test_bug1028200-6.html
@@ +66,5 @@
> + own task and not integrated with the above task (visual separation).
> +
> + =)
> + */
> + yield SimpleTest.promiseWaitForConditionWithReject(() => {
Let's go with the approach I laid out in test_bug1028200-2.html.
::: dom/plugins/test/mochitest/test_bug1028200-7.html
@@ +72,5 @@
> + yield SimpleTest.promiseWaitForConditionWithReject(() => {
> + return !document.mozFullScreenElement;
> + })
> + .then(() => {
> + ok(false, "Element is still fullscreen");
As above (test_bug1028200-1.html approach)
::: testing/mochitest/tests/SimpleTest/SimpleTest.js
@@ +1022,5 @@
> /**
> + * Same as waitForCondition but uses resolve and reject instead of `aCallback`
> + * and `aErrormsg` .
> + */
> +SimpleTest.promiseWaitForConditionWithReject = function (aCond) {
Probably no longer necessary.

Along with the nits I found above, I had to add fullscreenchange event listeners to the cleanup functions to ensure that fullscreenchange state changes didn't leak between tests.
ni'ing myself to check for green results from comment 60 - if so, we can land!

Garrr, I forgot that we don't have the crash reporter on ASAN builds, so we don't get crash reports, so SimpleTest.expectChildProcessCrash is complaining.
The simplest way forward is probably just to disable the tests for the ASAN builds. I'll re-land with that.