1028200 - Crashed plugins should exit full-screen mode.

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Justin Dolske [:Dolske]]

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.

Comment 1

[Benjamin Smedberg]

Was this Firefox fullscreen mode or Flash fullscreen mode? Normally Flash fullscreen doesn't interact with mozFullScreenElement and is not really visible to Firefox.

Comment 2

[Justin Dolske [:Dolske]]

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.

Comment 3

[Benjamin Smedberg]

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.

Comment 4

[Sushrut Bhalla]

Hi,

I am new here and would like to take this bug.

Thanks,

Sushrut

Comment 5

[Benjamin Smedberg]

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 6

[Aman Alam]

Attachment: A proposed patch for the Flash full screen issue

Comment 7

[Benjamin Smedberg]

Comment on attachment 8485458 [details] [diff] [review]
A proposed patch for the Flash full screen issue

Setting review for gfritzsche. I expect we'd at least want a comment explaining this.

Comment 8

[Georg Fritzsche [:gfritzsche]]

Sorry for the delay Amam, i got held up by other tasks. I plan to check this out tomorrow.

Comment 9

[Georg Fritzsche [:gfritzsche]]

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 10

[sssarvjeet27]

could you please assign this one to me?

Comment 11

[sssarvjeet27]

Attachment: bud-1028200-fix.patch

just before displaying the crash notice, check if in full screen mode if yes cancel full screen.

Comment 12

[Benjamin Smedberg]

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 13

[sssarvjeet27]

Attachment: bug-1028200-fix-v2.patch

Comment 14

[Benjamin Smedberg]

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 15

[sssarvjeet27]

Attachment: bug-1028200-fix.patch

Comment 16

[Benjamin Smedberg]

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.

Comment 17

[sfballais123]

Writing the test is interesting. I think I'll tackle writing the test.

Comment 18

[s7hoang]

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.

Comment 19

[Benjamin Smedberg]

s7hoang: Are you using the Firefox developer tools? PluginContent.jsm is a part of Firefox: it doesn't run as part of the webpage. So you'd have to enable the pref to debug Firefox chrome, and then launch the Browser Toolbox. See https://developer.mozilla.org/en-US/docs/Tools/Browser_Toolbox for details.

Comment 20

[s7hoang]

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.

Comment 21

[s7hoang]

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".

Comment 22

[Benjamin Smedberg]

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.

Comment 23

[s7hoang]

Attachment: 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 24

[s7hoang]

That's too bad.
I managed to get around it by using `mozCancelFullScreen()` for my feedback though :)
Turns out PluginContent.jsm was actually running after all.

Comment 25

[s7hoang]

Attachment: patch_base=changeset:308747_rolled

Found out about histedit rolling in mercurial.  Thought it might be nicer to have one clean commit to review.

Comment 26

[Benjamin Smedberg]

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.

Comment 27

[s7hoang]

Attachment: 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 28

[s7hoang]

Attachment: Style + Algorithm Change

forgot to include style changes to test_bug1028200.html

Comment 29

[s7hoang]

Attachment: Style + Algorithm Change

Couldn't get eslint to lint test_bug1028200.html so I did it manually. I think I got them all this time.

Comment 30

[Benjamin Smedberg]

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

Comment 31

[s7hoang]

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.

Comment 32

[Benjamin Smedberg]

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.

Comment 33

[s7hoang]

In terms of writing a test, how do I write a cross-origin test?

I can't set "src" to any actual website because it immediately kills the browser.

Comment 34

[Benjamin Smedberg]

For mochitests the same content is served from different domains. See https://developer.mozilla.org/en-US/docs/Mozilla/Projects/Mochitest#SSL and https://dxr.mozilla.org/mozilla-central/source/build/pgo/server-locations.txt

So you could do example.org as your cross-domain <iframe>

Comment 35

[s7hoang]

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.

Comment 36

[Mike Conley (:mconley) (:⚙️)]

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?

Comment 37

[s7hoang]

Oh cool, `candidate.ownerGlobal.frameElement` was what I was trying to get to this whole time, just didn't know how.

That helps plenty thanks!

Comment 38

[s7hoang]

Is there a naming scheme or other form of organization for the tests?

I'm guessing test_bug1028200_1, test_bug1028200_2 - and so on - isn't a good way of going about it?

Comment 39

[s7hoang]

Nevermind, I found some tests that are named in exactly this way :P

Comment 40

[Benjamin Smedberg]

Yep, you can use bug numbers. You can also use functional names like test_fullscreen_crash.html

Comment 41

[s7hoang]

Attachment: bug1028200_2.diff

Comment 42

[s7hoang]

Attachment: bug1028200_2.diff

Comment 43

[s7hoang]

Attachment: 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 44

[Benjamin Smedberg]

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.

Comment 45

[s7hoang]

Attachment: bug1028200_2.diff

I don't know if this matters but revision 308747 was my base.

Comment 46

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8801439 [details] [diff] [review]
bug1028200_2.diff

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

Thanks s7hoang! I have some comments and suggestions. Let me know if you have any questions!

::: browser/modules/PluginContent.jsm
@@ +899,4 @@
>     * target of the event is the document that the GMP is being used in.
>     */
>    onPluginCrashed: function (target, aEvent) {
> +    function isCrashedPluginContainedWithinFullscreenElement(_this, fullScreenElement, crashedPlugin) {

I like the spirit of this function, but I think the name might be a bit verbose. Plus, I think it's more common to assign functions like this to a variable when we're using them within another function. What I suggest:


/**
 * Documentation for what this helper function does
 * 
 * @param fullScreenEl (DOMElement)
 *        Description of what this element is used for.
 * @param plugin (DOMElement hosting a plugin)
 *        Description of what this element is used for.
 * @returns bool
 *        Description of what this return value means.
 */
let isWithinFullScreenElement = (fullScreenEl, plugin) => {
  // ...
};

Or, maybe even better, define this function on the PluginContent.prototype itself.

Then:

let fullScreenElement = this.content.document.mozFullScreenElement
if (fullScreenElement && isWithinFullScreenElement(fullScreenElement, target)) {
  this.content.document.mozCancelFullScreen();
}

If you end up defining the function on PluginContent.prototype, the above becomes:

let fullScreenElement = this.content.document.mozFullScreenElement
if (fullScreenElement && this.isWithinFullScreenElement(fullScreenElement, target)) {
  this.content.document.mozCancelFullScreen();
}

@@ +899,5 @@
>     * target of the event is the document that the GMP is being used in.
>     */
>    onPluginCrashed: function (target, aEvent) {
> +    function isCrashedPluginContainedWithinFullscreenElement(_this, fullScreenElement, crashedPlugin) {
> +      if (fullScreenElement.tagName === "IFRAME") {

traverseDownwardToTrueFullScreenElement is another function name that we can probably shorten. We should probably put its definition closer to where it's used. Example:

let getTrueFullScreenElement = (el) => {
  if (!el) {
    return null;
  }

  if (el.tagName != "IFRAME") {
    return el;
  }

  // We're dealing with an iframe at this point.

  if (!el.contentDocument) {
    // We reached in iframe that somehow doesn't have a document
    // associated with it.
    return el;
  }

  if (!el.contentDocument.mozFullScreenElement) {
    // We reached an iframe that doesn't have a fullScreenElement, which
    // means that the iframe itself must be the fullScreenElement. In that
    // case, return the contentDocument for the iframe, since that's what
    // we'll be searching for the crashed plugin.
    return el.contentDocument;
  }

  // We must have an iframe that is hosting a document that has its
  // own mozFullScreenElement, so let's go down another level.
  return getTrueFullScreenElement(el.contentDocument.mozFullScreenElement);
};

fullScreenElement = getTrueFullScreenElement(fullScreenElement);
if (!fullScreenElement) {
  // We should probably Cu.reportError this and then bail.
}

@@ +907,5 @@
> +      if (fullScreenElement.contains(crashedPlugin)) {
> +        return true;
> +      }
> +
> +      let parentIframe = crashedPlugin.ownerGlobal.frameElement;

Not sure this step is really necessary anymore.

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?

@@ +909,5 @@
> +      }
> +
> +      let parentIframe = crashedPlugin.ownerGlobal.frameElement;
> +      if (parentIframe) {
> +        return isCrashedPluginContainedWithinFullscreenElement(_this, fullScreenElement, parentIframe);

Note that _this is never used. You can drop it.

@@ +946,4 @@
>        // this crash, so we should hold off showing the crash report
>        // UI.
>        return;
> +      }

The alignment looks wrong for this line down.

::: dom/plugins/test/mochitest/1028200-subpageC.html
@@ +3,5 @@
> +    <meta charset="utf-8" />
> +  </head>
> +<body>
> +  <h1>1028200 subpageC</h1>
> +  <div id="div1"> </div>

Some of these tags aren't closed. Is that intentional? (True for several of the other subpage files as well)

::: dom/plugins/test/mochitest/test_bug1028200-1.html
@@ +3,5 @@
> +  <title>Plugin Crash, FullScreenElement Cancelled, div -> iframe -> iframe -> plugin</title>
> +  <script type="application/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>
> +  <script type="application/javascript" src="plugin-utils.js"></script>
> +
> +<body onLoad="window.frameLoaded()">

Indentation is wrong here, and in other parts of this file (example: lines 9-14, lines 18 - 70)

@@ +20,5 @@
> +
> +  var fullScreenElement = document.getElementById('div1');
> +  var plugin = document.getElementById('iframe1').contentDocument.getElementById('iframe2').contentDocument.getElementById('plugin1');
> +
> +  var beginTest = function (fullScreenElement, plugin) {

I like that you're using Promises here. You might want to use SpawnTask instead.

You can add it via:

<script type="text/javascript" src="/tests/SimpleTest/SpawnTask.js"></script>

Example usage: http://searchfox.org/mozilla-central/search?q=SpawnTask&=mozilla-central&redirect=true

@@ +27,5 @@
> +
> +      /* needs a moment to take effect */
> +      window.setTimeout(function() {
> +        resolve();
> +      }, 100);

This is going to be really race-y, and may lead to intermittent test failures.

It'd probably be better to wait for the mozfullscreenchange event instead.

@@ +34,5 @@
> +    promise.then(function() {
> +      if (document.mozFullScreenElement) {
> +        ok(true, "FullScreenElement is fullscreen");
> +        try {
> +          plugin.crash();

You should probably set up a mozfullscreenchange event listener before you crash, and then have a Promise resolve once it fires after you crash the plugin.

@@ +57,5 @@
> +      }, 500);
> +    });
> +
> +    promise.then(function() {
> +      (document.mozFullScreenElement) ?

This can probably just be simplified to:

ok(!document.mozFullScreenElement, "Not in fullscreen anymore");

::: dom/plugins/test/mochitest/test_bug1028200-2.html
@@ +14,5 @@
> +SpecialPowers.setBoolPref("full-screen-api.allow-trusted-requests-only", false);
> +  </script>
> +
> +  <script class="testbody" type="application/javascript">
> +window.frameLoaded = function testCrashChildPlugin_expectFullScreenElementToStay() {

These tests are all very similar to one another. What I'd recommend is factoring out the common functions into dom/plugins/test/mochitest/head.js and sharing them between the tests.

Comment 47

[s7hoang]

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?

Comment 48

[s7hoang]

Attachment: 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.

Comment 49

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8805021 [details] [diff] [review]
bug1028200_3.diff

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

Thanks for the patch, s7hoang! The fix itself looks pretty good, just a few minor comments there. I have some larger-scale feedback about the tests. Please let me know if you have any questions!

::: browser/modules/PluginContent.jsm
@@ +900,5 @@
>     */
>    onPluginCrashed: function (target, aEvent) {
> +
> +    /**
> +     * Determines whether or not the crashed plugin is contained within current full screen DOM element.

I'm sorry, I should have been more clear about what I meant about assigning this function to the prototype:

Can you make isWithinFullScreenElement a sibling of the onPluginCrashed method that we're inside? Like, we have:

PluginContent.prototype = {
  // ...
  onPluginCrashed: function (target, aEvent) {
    // ...
  },

  isWithinFullScreenElement: function (el, plugin) {
    // ...
  },

}

@@ +930,5 @@
> +      }
> +
> +      if (fullScreenElement.contains(crashedPlugin)) {
> +        return true;
> +      }

} else {

@@ +934,5 @@
> +      }
> +      else{
> +        let parentIframe = crashedPlugin.ownerGlobal.frameElement;
> +        if (parentIframe) {
> +          return this.isWithinFullScreenElement(fullScreenElement, parentIframe);

We're re-using the crashedPlugin to search the fullScreenElement for the parentIframe... so I think our documentation is wrong now. Can you update it for what this second argument represents? Either a crashed plugin or something that contains a crashed plugin?

::: dom/plugins/test/mochitest/1028200-head.js
@@ +1,1 @@
> +SimpleTest.waitForExplicitFinish();

Instead of adding a new 1028200-head.js, can we not just put these inside the pre-existing head.js? They'll also need to be properly documented.

@@ +1,3 @@
> +SimpleTest.waitForExplicitFinish();
> +setTestPluginEnabledState(SpecialPowers.Ci.nsIPluginTag.STATE_ENABLED);
> +SimpleTest.requestFlakyTimeout("The element needs to be fullscreen before \

We should avoid setTimeout at all costs. See below.

@@ +33,5 @@
> +function checkCancelledFullScreen() {
> +  var promise = new Promise (function(resolve) {
> +    /* Needs a longer moment for `mozCancelFullScreen()` to take effect from
> +     * `PluginContent.jsm` */
> +    window.setTimeout(function() {

We should not be using setTimeout to wait for the transition to complete. This is race-y, and will lead to fragile tests that fail intermittently.

We should instead wait for the fullscreenchange event here.

@@ +41,5 @@
> +
> +  promise.then(function() {
> +    ok(!document.mozFullScreenElement,
> +        "Element is not fullscreen anymore");
> +    SimpleTest.finish();

This will finish the running test, which seems a bit outside of the responsibility of a helper function. Perhaps we should just return the Promise.

@@ +63,5 @@
> +}
> +
> +function cancelFullScreenForConvenience() {
> +  var promise = new Promise (function(resolve, reject) {
> +    SimpleTest.finish();

Why is the test finishing here?

@@ +65,5 @@
> +function cancelFullScreenForConvenience() {
> +  var promise = new Promise (function(resolve, reject) {
> +    SimpleTest.finish();
> +    /* Wait some time for the test to capture the results before proceeding */
> +    window.setTimeout(function() {

Same as above, regarding setTimeout. We cannot use these to wait for transitions to complete.

::: 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>

These tests aren't using SpawnTask, but should - particularly because your helpers return Promises.

@@ +4,5 @@
> +  <script type="application/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>
> +  <script type="application/javascript" src="plugin-utils.js"></script>
> +</head>
> +
> +<body onLoad="window.frameLoaded()">

To avoid confusion regarding which frames are loading, we should probably just call this function "load", like so:

<body onLoad="load();">
...
function load() {
  let full...
}
...
</body>

Same goes for the other window.frameLoaded usage.

Also, you should be using add_task from SpawnTask here instead, and yielding Promises. See http://searchfox.org/mozilla-central/rev/021e8e0cee4f446757b8b1ffd312272174d6fc7b/toolkit/content/tests/mochitest/test_autocomplete_change_after_focus.html#11 as an example.

@@ +15,5 @@
> +}
> +  </script>
> +
> +  <div id="div1">
> +    <iframe id="iframe1" src="1028200-subpageA.html" height=600 width=600></iframe>

All attributes should be in quotes.

Comment 50

[s7hoang]

Attachment: 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 51

[Mike Conley (:mconley) (:⚙️)]

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?

Comment 52

[s7hoang]

Attachment: 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".

Comment 53

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8812542 [details] [diff] [review]
bug1028200_5.diff

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

So I dug into this, and I think I've found at least part of the problem. The problem is that I misled you, and I apologize for that - I had thought that head.js was being shared between all mochitests within this directory, but looking at the manifest (http://searchfox.org/mozilla-central/rev/59bb309e38b10aba63dea8505fb800e99fe821d6/dom/plugins/test/mochitest/mochitest.ini) that doesn't appear to be the case. Notice how head.js is missing from this file. It is, however, included in http://searchfox.org/mozilla-central/rev/59bb309e38b10aba63dea8505fb800e99fe821d6/dom/plugins/test/mochitest/browser.ini. So what that means is that the functions in head.js are only available to the mochitest-browser type tests, which isn't what your tests are (they're type "mochitest-plain").

It looks like there's another file that's being used for utilities, called plugin_utils.js. This is where utility functions are being stored that are shared between tests of type mochitest-plain and mochitest-chrome. Here's an example of it being included: http://searchfox.org/mozilla-central/rev/59bb309e38b10aba63dea8505fb800e99fe821d6/dom/html/test/file_fullscreen-plugins.html#19

So, my sincere apologies for leading you down the garden path with the head.js suggestion. That was the wrong one, and I wasted your time. Sorry about that.

What we should do instead is move the utility functions to plugin_utils.js.

Big thumbs up on all the tests you're writing here - it's really bolstering my confidence that your PluginContent.jsm patch is solid.

::: dom/plugins/test/mochitest/head.js
@@ +131,5 @@
>    return null;
>  }
>  
> +/**
> + * Adds a one-time event listener on `mozFullScreenChange` with `action`.

Thanks for the great documentation!

@@ +134,5 @@
> +/**
> + * Adds a one-time event listener on `mozFullScreenChange` with `action`.
> + * @param action	Routine to execute upon triggering.
> + */
> +function doOnFullScreenChangeOneTime(action){

Instead of calling the "action" callback, can you make this function return a Promise that resolves once the fullscreenchange event fires?

::: dom/plugins/test/mochitest/test_bug1028200-1.html
@@ +6,5 @@
> +  <script type="application/javascript" src="plugin-utils.js"></script>
> +</head>
> +
> +<body onLoad="load()">
> +  <script class="testbody" type="application/javascript" src="head.js"></script>

We won't need this when we move the helpers to plugin-utils.js.

@@ +15,5 @@
> +    /* Needed to be able to programatically (without user interaction) enter
> +     * fullscreen  (has been deemed a security issue otherwise and therefore
> +     * disabled by default)
> +    */
> +    SpecialPowers.setBoolPref("full-screen-api.allow-trusted-requests-only", false);

This pref isn't being cleared. You should use SpecialPowers.pushPrefEnv instead, which will cause the pref to be cleared once the test exists (on success or failure).

Here's an example of it being used:

http://searchfox.org/mozilla-central/rev/59bb309e38b10aba63dea8505fb800e99fe821d6/dom/base/test/test_mozbrowser_apis_blocked.html#14

@@ +17,5 @@
> +     * disabled by default)
> +    */
> +    SpecialPowers.setBoolPref("full-screen-api.allow-trusted-requests-only", false);
> +
> +    load = function testCrashChildPlugin_expectFullScreenElementToBeCancelled() {

Can you please add a docstring above this test function describing what is being tested?

Also, load is missing a let.

@@ +24,5 @@
> +        let plugin = document.getElementById('iframe1')
> +          .contentDocument.getElementById('iframe2')
> +          .contentDocument.getElementById('plugin1');
> +
> +        yield new Promise (resolve => {

If you make doOnFullScreenChangeOneTime return a Promise as I suggested earlier, then this becomes:

let fullscreenPromise = doOnFullScreenChangeOneTime(); // or whatever we end up calling it
fullScreenElement.mozRequestFullScreen();
yield fullscreenPromise;
// Once we re-enter, we've entered fullscreen!

@@ +32,5 @@
> +          });
> +          fullScreenElement.mozRequestFullScreen();
> +        });
> +
> +        yield crashPlugin(plugin)

If the Promise rejects, or something within it throws, I believe TaskSpawn should cause the test to fail automatically, so I don't think the catch is necessary.

The then() is also not necessary. You can do this:

yield crashPlugin(plugin);
ok(true, "Plugin crashed");

@@ +38,5 @@
> +          .then (() => { ok(true , "Plugin was crashed"); });
> +      });
> +
> +      add_task(function* (){
> +        yield new Promise(resolve => {

Why is the new add_task necessary? Can we not just combine the yields with the add_task above?

Like:

let crashFullScreenChange = doOnFullScreenChangeOneTime(); // or whatever we end up calling it
yield crashPlugin(plugin);
yield crashFullScreenChange;
// If you'd like, you could check to see if mozFullScreenElement is null here without doing the polling.
ok(!document.mozFullScreenElement, "There shouldn't be a fullscreen element anymore");

@@ +43,5 @@
> +          let onFullScreen = doOnFullScreenChangeOneTime(() => {
> +            ok(true, "Element is no longer fullscreen");
> +            resolve();
> +          });
> +          SimpleTest.promiseWaitForConditionWithReject(() => {

Why is this check necessary with polling? Shouldn't the mozFullScreenElement be cleared after fullscreenchange is fired?

It looks like the doOnFullScreenChangeOneTime Promise isn't being used to it's full potential here. See my above comment.

::: dom/plugins/test/mochitest/test_bug1028200-2.html
@@ +6,5 @@
> +  <script type="application/javascript" src="plugin-utils.js"></script>
> +</head>
> +
> +<body onLoad="load()">
> +  <script class="testbody" type="application/javascript" src="head.js"></script>

No longer necessary once the utilites are moved to plugin-utils.js.

@@ +15,5 @@
> +    /* Needed to be able to programatically (without user interaction) enter
> +     * fullscreen  (has been deemed a security issue otherwise and therefore
> +     * disabled by default)
> +    */
> +    SpecialPowers.setBoolPref("full-screen-api.allow-trusted-requests-only", false);

See my other note re: pushPrefEnv.

@@ +21,5 @@
> +    /* FullScreen element is expected to remain alive after the test ends; this
> +     * stops it.
> +     */
> +    SimpleTest.registerCleanupFunction(() => {
> +      this.document.mozCancelFullScreen();

registerCleanupFunction can take a function that returns a Promise. We should call mozCancelFullScreen, and return a Promise once the fullscreenchange event fires to ensure that we've fully exited.

@@ +24,5 @@
> +    SimpleTest.registerCleanupFunction(() => {
> +      this.document.mozCancelFullScreen();
> +    });
> +
> +    load = function testCrashChildPlugin_expectFullScreenElementToRemain() {

Missing let on the "load", and a docstring describing what this test tests.

@@ +25,5 @@
> +      this.document.mozCancelFullScreen();
> +    });
> +
> +    load = function testCrashChildPlugin_expectFullScreenElementToRemain() {
> +      add_task(function* () {

A lot of my comments about the other test file apply to this one as well.

::: dom/plugins/test/mochitest/test_bug1028200-3.html
@@ +15,5 @@
> +    /* Needed to be able to programatically (without user interaction) enter
> +     * fullscreen  (has been deemed a security issue otherwise and therefore
> +     * disabled by default)
> +    */
> +    SpecialPowers.setBoolPref("full-screen-api.allow-trusted-requests-only", false);

Carry forward comments from other tests to this one.

::: dom/plugins/test/mochitest/test_bug1028200-4.html
@@ +15,5 @@
> +    /* Needed to be able to programatically (without user interaction) enter
> +     * fullscreen  (has been deemed a security issue otherwise and therefore
> +     * disabled by default)
> +    */
> +    SpecialPowers.setBoolPref("full-screen-api.allow-trusted-requests-only", false);

Carry forward comments from other tests to this one.

::: dom/plugins/test/mochitest/test_bug1028200-5.html
@@ +15,5 @@
> +    /* Needed to be able to programatically (without user interaction) enter
> +     * fullscreen  (has been deemed a security issue otherwise and therefore
> +     * disabled by default)
> +    */
> +    SpecialPowers.setBoolPref("full-screen-api.allow-trusted-requests-only", false);

Carry forward comments from other tests to this one.

::: dom/plugins/test/mochitest/test_bug1028200-6.html
@@ +16,5 @@
> +    /* Needed to be able to programatically (without user interaction) enter
> +     * fullscreen  (has been deemed a security issue otherwise and therefore
> +     * disabled by default)
> +    */
> +    SpecialPowers.setBoolPref("full-screen-api.allow-trusted-requests-only", false);

Carry forward comments from other tests to this one.

::: dom/plugins/test/mochitest/test_bug1028200-7.html
@@ +15,5 @@
> +    /* Needed to be able to programatically (without user interaction) enter
> +     * fullscreen  (has been deemed a security issue otherwise and therefore
> +     * disabled by default)
> +    */
> +    SpecialPowers.setBoolPref("full-screen-api.allow-trusted-requests-only", false);

Carry forward comments from other tests to this one.

::: testing/mochitest/tests/SimpleTest/SimpleTest.js
@@ +1021,5 @@
>  
>  /**
> + * Same as waitForCondition but uses resolve and reject instead of aCallback
> + * and aErrormsg* .
> + */

I don't think we need this utility, tbh. See my earlier comments.

Comment 54

[s7hoang]

Attachment: 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 55

[s7hoang]

Oh by the way I haven't found that `SpawnTask` does anything special on
`Promise.reject`.  So if you don't handle it yourself it'll give you `ok(false,
"undefined")`

Comment 56

[Mike Conley (:mconley) (:⚙️)]

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.

Comment 57

[s7hoang]

Attachment: bug1028200_7.diff

Comment 58

[s7hoang]

Attachment: bug1028200_7.diff

Forgot to fill in a comment

Comment 59

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8817930 [details] [diff] [review]
bug1028200_7.diff

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

Hey s7hoang, I have a few nits below, but I think this has gone through plenty of review. You've worked really hard on this stuff, and done a great job driving this over the line. I'll take care of fixing up these nits and doing a try push for you. If the push turns green, let's get this thing landed!

Thanks so much!

::: dom/plugins/test/mochitest/test_bug1028200-1.html
@@ +18,5 @@
> +        "having called `mozCancelFullScreen` without the timeout because it " +
> +        "will return the value prior to actually cancelling. A timeout is " +
> +        "preferred here as opposed to polling methods similar to " +
> +        "`SimpleTest.waitForCondition` in `SimpleTest.js` for reasons of" +
> +        "idiomaticity."

Great documentation, thank you!

@@ +21,5 @@
> +        "`SimpleTest.waitForCondition` in `SimpleTest.js` for reasons of" +
> +        "idiomaticity."
> +        );
> +
> +    /* FullScreen element is expected to remain alive after the test ends; this

Nit: Comment blocks should start like this

/**
 * First line

@@ +54,5 @@
> +        fullScreenElement.mozRequestFullScreen();
> +        yield fullScreenChange;
> +        ok(true, "Element is fullscreen");
> +
> +

Nit - let's remove this extra newline.

@@ +66,5 @@
> +      });
> +
> +      add_task(function* () {
> +        /*
> +           Poll for the fullscreen element still being fullscreen.  After

We're not polling anymore - we're using a setTimeout to check after some short period of time. We should update this comment.

Also, it should use:

/**
 * First line
 * second line
 * ...
 */

Formatting

::: dom/plugins/test/mochitest/test_bug1028200-2.html
@@ +52,5 @@
> +        fullScreenElement.mozRequestFullScreen();
> +        yield fullScreenChange;
> +        ok(true, "Element is fullscreen");
> +
> +

Unnecessary newline.

@@ +64,5 @@
> +      });
> +
> +      add_task(function* () {
> +        /*
> +           Poll for the fullscreen element still being fullscreen.  After

Technically we're not polling anymore.

Comment 60

[Mike Conley (:mconley) (:⚙️)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f5088f2cee81

Comment 61

[Mike Conley (:mconley) (:⚙️)]

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!

Comment 62

[Mike Conley (:mconley) (:⚙️)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=964e834142bf

Comment 63

[Mike Conley (:mconley) (:⚙️)]

Attachment: Plugins that are fullscreen'd should exit fullscreen mode on crash

MozReview-Commit-ID: HiDyiPCPiV1

Comment 64

[Mike Conley (:mconley) (:⚙️)]

Author: s7hoang <[e-mail]>
Bug number: 1028200
Commit message: "Bug 1028200 - Plugins that are fullscreen'd should exit fullscreen mode on crash. r=mconley"

Comment 65

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/843b98464b40
Plugins that are fullscreen'd should exit fullscreen mode on crash. r=mconley

Comment 66

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ccee8b226c3b
Backed out changeset 843b98464b40 for failing on own test

Comment 67

[Mike Conley (:mconley) (:⚙️)]

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.

Comment 68

[Mike Conley (:mconley) (:⚙️)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/da76054452f3306b159d30584d4307ac3d612485
Bug 1028200 - Plugins that are fullscreen'd should exit fullscreen mode on crash. r=mconley

Comment 69

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/da76054452f3

Comment 70

[Andrei Vaida [:avaida]]

Setting qe-verify- since this seems to have automated coverage. Mike, if you think manual QA should instead be looking at this, feel free to flip the flag.