474527 - onbeforeaccept handler in a <prefwindow> can't prevent the window from closing

Status: RESOLVED FIXED

(Toolkit :: Preferences, defect)

Description

[Robby Dermody]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.0.5) Gecko/2008120122 Firefox/3.0.5
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.0.5) Gecko/2008120122 Firefox/3.0.5

I have a custom FF extension I'm working on that has a tab in the FF preferences window. In preferences_overlay.xul I have a tag like:

<prefwindow id="BrowserPreferences" onbeforeaccept="return objPhLinkSettings.validateSettings();">

Then in validateSettings() I check the settings of the text fields to make sure the data entered is valid. If it isn't, I'd like to pop up an alert box stating so, **and allowing the user to correct the error and try again**. Going from the MDC docs, I should be able to just return false from this function. Doing this will cause the prefs not to be saved (good), but will lead to the Preferences window itself still being closed (bad), when it should stay open to allow the user to correct the error and try again.

Returning true from this validateSettings() function works as expected: the Preferences window closes and the prefs are saved.

I'll mark this as normal priority for now because I don't see a workaround to it. Unless there is some other way I could be validating preferences before allowing the prefs window to close...

Reproducible: Always

Steps to Reproduce:
1.
2.
3.


Expected Results:  
return false from the callback should keep the preferences window open and not save the prefs.

Comment 1

[:Gavin Sharp [email: [e-mail]]]

Attachment: patch

The current dialogaccept handler does allow beforeaccept handlers to prevent the prefs from being saved, but the window always closes regardless. This patch makes the return value propagate to dialog.xml's _fireButtonEvent, which will also cancel the window close.

I searched http://mxr-test.konigsberg.mozilla.org/addons/ (MXR for addons.mozilla.org) for addons that use this event, and most never return false and just use it to do cleanup or work. The ones that do return false (only 3 of them) look like they were intending it to prevent the dialog from closing, since they use it to validate the pref values and alert() when the validation fails.

Not sure who the best person to review this is, really...

Comment 2

[:Gavin Sharp [email: [e-mail]]]

For reference, the extensions that return false from an ondialogaccept handlers are:

http://mxr-test.konigsberg.mozilla.org/addons/source/6230/chrome/content/hellafox-prefs.js#2
http://mxr-test.konigsberg.mozilla.org/addons/source/4023/chrome/content/options.js#2
http://mxr-test.konigsberg.mozilla.org/addons/source/3788/chrome/content/AddonOptions.xul#128

Comment 3

[Neil Deakin]

Comment on attachment 357911 [details] [diff] [review]
patch

>+      // try again, this one should succeed
>+      prefWindow.document.documentElement.acceptDialog();
>+      is(prefWindow.closed, true, "window now closed");
>+      is(dialogShown.valueFromPreferences, true, "pref committed");
>+

You might want to check the other values again here just to make sure.

Comment 4

[:Gavin Sharp [email: [e-mail]]]

Attachment: updated patch [Backout: Comment 7]

Comment 5

[Serge Gautherie (:sgautherie)]

Comment on attachment 359189 [details] [diff] [review]
updated patch
[Backout: Comment 7]


http://hg.mozilla.org/mozilla-central/rev/6430d01da9e3

Comment 6

[Ben Turner (not reading bugmail, use the needinfo flag!)]

This failed unit tests on windows and was backed out:

 http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1234810582.1234815682.2911.gz

Comment 7

[Serge Gautherie (:sgautherie)]

Comment on attachment 359189 [details] [diff] [review]
updated patch
[Backout: Comment 7]


Backed out:
http://hg.mozilla.org/mozilla-central/rev/dc4f9eebe88b
http://hg.mozilla.org/mozilla-central/rev/14da8a8f1e4d

See for example:
{
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1234810582.1234815682.2911.gz
WINNT 5.2 mozilla-central unit test on 2009/02/16 10:56:22

*** 1075 ERROR TEST-UNEXPECTED-FAIL | chrome://mochikit/content/chrome/toolkit/content/tests/chrome/test_preferences_beforeaccept.xul | Test timed out.
}

Comment 8

[:Gavin Sharp [email: [e-mail]]]

Thanks Serge/Ben.

Comment 9

[Manish Goregaokar [:manishearth]]

Attachment: Patch 1.0

Attempted to fix patch by canceling the event bubble.

I have verified that this works with my patch for bug 388287, however I can't get the tests to run.

Comment 10

[Manish Goregaokar [:manishearth]]

Comment on attachment 797731 [details] [diff] [review]
Patch 1.0

># HG changeset patch
># Parent 44e615a66f3fbe54af80058fdac1814687255820
># User Manish Goregaokar <[e-mail]>
>Bug 474527 - onbeforeaccept handler in a <prefwindow> can't prevent the window from closing; r=MattN
>
>diff --git a/toolkit/content/tests/chrome/Makefile.in b/toolkit/content/tests/chrome/Makefile.in
>--- a/toolkit/content/tests/chrome/Makefile.in
>+++ b/toolkit/content/tests/chrome/Makefile.in
>@@ -57,16 +57,18 @@ MOCHITEST_CHROME_FILES = \
> 		test_autocomplete4.xul \
> 		test_autocomplete5.xul \
> 		test_autocomplete_delayOnPaste.xul \
> 		file_autocomplete_with_composition.js \
> 		test_autocomplete_with_composition_on_input.html \
> 		test_autocomplete_with_composition_on_textbox.xul \
> 		test_keys.xul \
> 		window_keys.xul \
>+		test_preferences_beforeaccept.xul \
>+		window_preferences_beforeaccept.xul \
> 		test_showcaret.xul \
> 		window_showcaret.xul \
> 		test_righttoleft.xul \
> 		test_dialogfocus.xul \
> 		dialog_dialogfocus.xul \
> 		test_screenPersistence.xul \
> 		window_screenPosSize.xul \
> 		test_titlebar.xul \
>diff --git a/toolkit/content/tests/chrome/test_preferences_beforeaccept.xul b/toolkit/content/tests/chrome/test_preferences_beforeaccept.xul
>new file mode 100644
>--- /dev/null
>+++ b/toolkit/content/tests/chrome/test_preferences_beforeaccept.xul
>@@ -0,0 +1,62 @@
>+<?xml version="1.0"?>
>+<?xml-stylesheet href="chrome://global/skin" type="text/css"?>
>+<?xml-stylesheet href="chrome://mochikit/content/tests/SimpleTest/test.css" type="text/css"?>
>+
>+<window title="Preferences Window Tests"
>+        xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul">
>+
>+  <script type="application/javascript" 
>+          src="chrome://mochikit/content/MochiKit/packed.js"/>
>+  <script type="application/javascript"
>+          src="chrome://mochikit/content/tests/SimpleTest/SimpleTest.js"/>
>+
>+  <script type="application/javascript">
>+  <![CDATA[
>+    SimpleTest.waitForExplicitFinish();
>+
>+    // No instant-apply for this test
>+    var prefB = Components.classes["@mozilla.org/preferences-service;1"]
>+                          .getService(Components.interfaces.nsIPrefBranch);
>+    prefB.setBoolPref("browser.preferences.instantApply", false);
>+
>+    var prefWindow = openDialog("window_preferences_beforeaccept.xul", "", "", windowOnload);
>+
>+    function windowOnload() {
>+      var dialogShown = prefWindow.document.getElementById("tests.beforeaccept.dialogShown");
>+      var called = prefWindow.document.getElementById("tests.beforeaccept.called");
>+      is(dialogShown.value, true, "dialog opened, shown pref set");
>+      is(dialogShown.valueFromPreferences, null, "shown pref not committed");
>+      is(called.value, null, "beforeaccept not yet called");
>+      is(called.valueFromPreferences, null, "beforeaccept not yet called, pref not committed");
>+
>+      // try to accept the dialog, should fail the first time
>+      prefWindow.document.documentElement.acceptDialog();
>+      is(prefWindow.closed, false, "window not closed");
>+      is(dialogShown.value, true, "shown pref still set");
>+      is(dialogShown.valueFromPreferences, null, "shown pref still not committed");
>+      is(called.value, true, "beforeaccept called");
>+      is(called.valueFromPreferences, null, "called pref not committed");
>+
>+      // try again, this one should succeed
>+      prefWindow.document.documentElement.acceptDialog();
>+      is(prefWindow.closed, true, "window now closed");
>+      is(dialogShown.valueFromPreferences, true, "shown pref committed");
>+      is(called.valueFromPreferences, true, "called pref committed");
>+
>+      // clean up after ourselves
>+      prefB.clearUserPref("browser.preferences.instantApply");
>+      prefB.clearUserPref("tests.beforeaccept.dialogShown");
>+      prefB.clearUserPref("tests.beforeaccept.called");
>+
>+      SimpleTest.finish();
>+    }
>+  ]]>
>+  </script>
>+
>+  <body xmlns="http://www.w3.org/1999/xhtml">
>+    <p id="display"></p>
>+    <div id="content" style="display: none"></div>
>+    <pre id="test"></pre>
>+  </body>
>+
>+</window>
>diff --git a/toolkit/content/tests/chrome/window_preferences_beforeaccept.xul b/toolkit/content/tests/chrome/window_preferences_beforeaccept.xul
>new file mode 100644
>--- /dev/null
>+++ b/toolkit/content/tests/chrome/window_preferences_beforeaccept.xul
>@@ -0,0 +1,45 @@
>+<?xml version="1.0"?>
>+<?xml-stylesheet href="chrome://global/skin" type="text/css"?>
>+<!--
>+  XUL Widget Test for preferences window
>+-->
>+<prefwindow xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"
>+            title="preferences window"
>+            width="300" height="300"
>+            windowtype="test:preferences"
>+            buttons="accept,cancel"
>+            onbeforeaccept="return beforeAccept();"
>+            onload="onDialogLoad();"
>+>
>+  <script type="application/javascript">
>+  <![CDATA[
>+    function onDialogLoad() {
>+      var pref = document.getElementById("tests.beforeaccept.dialogShown");
>+      pref.value = true;
>+
>+      // call the onload handler we were passed
>+      window.arguments[0]();
>+    }
>+
>+    function beforeAccept() {
>+      var beforeAcceptPref = document.getElementById("tests.beforeaccept.called");
>+      var oldValue = beforeAcceptPref.value;
>+      beforeAcceptPref.value = true;
>+
>+      return !!oldValue;
>+    }
>+  ]]>
>+  </script>
>+
>+  <prefpane id="sample_pane" label="Sample Prefpane">
>+    <preferences id="sample_preferences">
>+      <preference id="tests.beforeaccept.called"
>+                  name="tests.beforeaccept.called"
>+                  type="bool"/>
>+      <preference id="tests.beforeaccept.dialogShown"
>+                  name="tests.beforeaccept.dialogShown"
>+                  type="bool"/>
>+    </preferences>
>+  </prefpane>
>+  <label>Test Prefpane</label>
>+</prefwindow>
>diff --git a/toolkit/content/widgets/preferences.xml b/toolkit/content/widgets/preferences.xml
>--- a/toolkit/content/widgets/preferences.xml
>+++ b/toolkit/content/widgets/preferences.xml
>@@ -1026,19 +1026,21 @@
>           return win;
>         ]]>
>         </body>
>       </method>
>     </implementation>
>     <handlers>
>       <handler event="dialogaccept">
>       <![CDATA[
>-        if (!this._fireEvent("beforeaccept", this)) 
>-          return;
>-        
>+        if (!this._fireEvent("beforeaccept", this)){
>+          return false;
>+          event.stopPropagation();
>+          event.preventDefault();        
>+        }
>         if (this.type == "child" && window.opener) {
>           var psvc = Components.classes["@mozilla.org/preferences-service;1"]
>                                .getService(Components.interfaces.nsIPrefBranch);
>           var instantApply = psvc.getBoolPref("browser.preferences.instantApply");
>           if (instantApply) {
>             var panes = this.preferencePanes;
>             for (var i = 0; i < panes.length; ++i)
>               panes[i].writePreferences(true);
>@@ -1091,16 +1093,18 @@
>           var panes = this.preferencePanes;
>           for (var i = 0; i < panes.length; ++i)
>             panes[i].writePreferences(false);
> 
>           var psvc = Components.classes["@mozilla.org/preferences-service;1"]
>                                .getService(Components.interfaces.nsIPrefService);
>           psvc.savePrefFile(null);
>         }
>+
>+        return true;
>       ]]>
>       </handler>
>       <handler event="command">
>         if (event.originalTarget.hasAttribute("pane")) {
>           var pane = document.getElementById(event.originalTarget.getAttribute("pane"));
>           this.showPane(pane);
>         }
>       </handler>

Comment 11

[Manish Goregaokar [:manishearth]]

Test passed (I ran ./mach mochitest-chrome toolkit/content/tests/chrome/test_preferences_beforeaccept.xul)


 0:35.30 15 INFO TEST-START | Shutdown
 0:35.30 16 INFO Passed:  12
 0:35.30 17 INFO Failed:  0
 0:35.30 18 INFO Todo:    0
 0:35.30 19 INFO Slowest: 1873ms - chrome://mochitests/content/chrome/toolkit/content/tests/chrome/test_preferences_beforeaccept.xul
 0:35.30 20 INFO SimpleTest FINISHED
 0:35.73 21 INFO TEST-INFO | Ran 1 Loops
 0:35.73 22 INFO SimpleTest FINISHED
 0:37.06 INFO | automation.py | Application ran for: 0:00:27.808761
 0:37.06 INFO | zombiecheck | Reading PID log: /tmp/tmphJCttXpidlog
 0:37.10 WARNING | leakcheck | refcount logging is off, so leaks can't be detected!
Mochitest INFO | runtests.py | Running tests: end.

Comment 12

[Manish Goregaokar [:manishearth]]

Attachment: Patch 1.1

Changed patch author, because the bulk of the changes are Gavin's.

Comment 13

[Manish Goregaokar [:manishearth]]

Attachment: Patch 1.2

Whoops, had mixed up some of the code.


Interestingly, the tests work fine on my computer with or without any part/all of the propagation prevention code.

Is there a way to test this on WINNT?

Comment 14

[Manish Goregaokar [:manishearth]]

Attachment: Patch 1.3

(qrefreshed on wrong patch stack)

Comment 15

[Matthew N. [:MattN]]

Comment on attachment 804453 [details] [diff] [review]
Patch 1.3

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

(In reply to Manish Goregaokar [:manishearth] from comment #13)
> Created attachment 804434 [details] [diff] [review]

There is an "x" at the end of Gavin's name in the user header of the patch.

> Interestingly, the tests work fine on my computer with or without any
> part/all of the propagation prevention code.
>
> Is there a way to test this on WINNT?

Which operating system are you testing on? Do you not have access to Windows? I have pushed this to try server[1] to see if it passes on Windows with the three combinations:
event.stopPropagation - https://tbpl.mozilla.org/?tree=Try&rev=a36361432c9d
event.preventDefault  - https://tbpl.mozilla.org/?tree=Try&rev=abeb8588abcc
Both                  - https://tbpl.mozilla.org/?tree=Try&rev=f9810c6f793c

Manish, can you check the results sometime after a few hours? I am on vacation this week so won't have time to watch the results.

::: toolkit/content/tests/chrome/test_preferences_beforeaccept.xul
@@ +4,5 @@
> +
> +<window title="Preferences Window Tests"
> +        xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul">
> +
> +  <script type="application/javascript" 

Nit: trailing whitespace

::: toolkit/content/widgets/preferences.xml
@@ +1032,5 @@
>        <handler event="dialogaccept">
>        <![CDATA[
> +        if (!this._fireEvent("beforeaccept", this)){
> +          event.stopPropagation();
> +          event.preventDefault();        

Nit: trailing whitespace

Comment 16

[Matthew N. [:MattN]]

Comment on attachment 804453 [details] [diff] [review]
Patch 1.3

Manish, were you able to see the try server results when they were still available? Do you have an updated patch?

Comment 17

[Manish Goregaokar [:manishearth]]

(In reply to Matthew N. [:MattN] from comment #16)
> Comment on attachment 804453 [details] [diff] [review]
> Patch 1.3
> 
> Manish, were you able to see the try server results when they were still
> available? Do you have an updated patch?

Oh, I think I forgot to comment about this (Sorry about that). All three seemed to work pretty fine, with some unrelated test timeouts for two of them. I believe the second one had no failures.

Comment 18

[Manish Goregaokar [:manishearth]]

So, should we investigate the test timeouts by resubmitting to the tryserver? Or just go ahead with event.preventDefault ?

Comment 19

[Matthew N. [:MattN]]

If we don't need to use preventDefault or stopProgation, I don't think we should use them. Here is a try push addressing my review comments with some other test cleanup fixes. If it's green, I'll push it.

Try push: https://tbpl.mozilla.org/?tree=Try&rev=a96df034bf76

Comment 20

[Manish Goregaokar [:manishearth]]

Windows XP 32-bit try opt test mochitest-2

TEST-UNEXPECTED-FAIL | /tests/dom/imptests/html/dom/ranges/test_Range-mutations.html | application timed out after 330 seconds with no output 
TEST-UNEXPECTED-FAIL | /tests/dom/imptests/html/dom/ranges/test_Range-mutations.html | application terminated with exit code 3221225477 
PROCESS-CRASH | /tests/dom/imptests/html/dom/ranges/test_Range-mutations.html | application crashed [@ CrashingThread(void *)] 
Return code: 1 


Rev3 Fedora 12x64 try debug test mochitest-browser-chrome


TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/social/browser_blocklist.js | unable to add provider Error: SocialService.addProvider: provider with this origin already exists
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/social/browser_blocklist.js | blocklist dialog was closed
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/social/browser_blocklist.js | runSocialTests: finish test run with 1 providers - Got 0, expected 1
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/social/browser_chat_tearoff.js | all provider prefs uninstalled from previous test - Got 1, expected 0
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/social/browser_chat_tearoff.js | all providers uninstalled from previous test 1 - Got 1, expected 0
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/social/browser_defaults.js | Exception thrown - [Exception... "Component returned failure code: 0x8000ffff (NS_ERROR_UNEXPECTED) [nsIPrefBranch.getComplexValue]" nsresult: "0x8000ffff (NS_ERROR_UNEXPECTED)" location: "JS frame :: chrome://mochitests/content/browser/browser/base/content/test/social/browser_defaults.js :: test :: line 11" data: no]
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/social/browser_share.js | all provider prefs uninstalled from previous test - Got 1, expected 0
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/social/browser_share.js | all providers uninstalled from previous test 1 - Got 1, expected 0
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/social/browser_social_activation.js | Test timed out
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/social/browser_social_activation.js | Found a tab after previous test timed out: https://example.com/browser/browser/base/content/test/social/social_activate.html
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/social/browser_social_activation.js | Found a tab after previous test timed out: about:addons
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/social/browser_social_chatwindow.js | Exception thrown at resource://gre/modules/SocialService.jsm:405 - Error: SocialService.addProvider: provider with this origin already exists
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/social/browser_social_chatwindow_resize.js | Exception thrown at resource://gre/modules/SocialService.jsm:405 - Error: SocialService.addProvider: provider with this origin already exists
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/social/browser_social_chatwindowfocus.js | uncaught exception - Error: SocialService.addProvider: provider with this origin already exists at resource://gre/modules/SocialService.jsm:405
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/social/browser_social_chatwindowfocus.js | Test timed out
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/social/browser_social_errorPage.js | Exception thrown at resource://gre/modules/SocialService.jsm:405 - Error: SocialService.addProvider: provider with this origin already exists
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/social/browser_social_flyout.js | Exception thrown at resource://gre/modules/SocialService.jsm:405 - Error: SocialService.addProvider: provider with this origin already exists
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/social/browser_social_isVisible.js | Exception thrown at resource://gre/modules/SocialService.jsm:405 - Error: SocialService.addProvider: provider with this origin already exists
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/social/browser_social_marks.js | Exception thrown at resource://gre/modules/SocialService.jsm:405 - Error: SocialService.addProvider: provider with this origin already exists
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/social/browser_social_multiprovider.js | Exception thrown at resource://gre/modules/SocialService.jsm:405 - Error: SocialService.addProvider: provider with this origin already exists
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/social/browser_social_multiworker.js | Exception thrown at resource://gre/modules/SocialService.jsm:405 - Error: SocialService.addProvider: provider with this origin already exists
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/social/browser_social_perwindowPB.js | Exception thrown at resource://gre/modules/SocialService.jsm:405 - Error: SocialService.addProvider: provider with this origin already exists
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/social/browser_social_sidebar.js | Exception thrown at resource://gre/modules/SocialService.jsm:405 - Error: SocialService.addProvider: provider with this origin already exists
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/social/browser_social_status.js | Exception thrown at resource://gre/modules/SocialService.jsm:405 - Error: SocialService.addProvider: provider with this origin already exists
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/social/browser_social_window.js | needs zero providers to start this test. - Got 1, expected 0
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/social/browser_social_window.js | no providers are enabled
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/social/browser_social_window.js | sub-test testEnabledStartup failed: Error: SocialService.addProvider: provider with this origin already exists
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/social/browser_social_window.js | sub-test testDisabledStartup failed: Error: SocialService.addProvider: provider with this origin already exists
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/social/browser_social_window.js | sub-test testRemoveProvider failed: Error: SocialService.addProvider: provider with this origin already exists
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/social/browser_social_window.js | all provider prefs uninstalled from previous test - Got 2, expected 0
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/social/browser_social_window.js | all providers uninstalled from previous test 1 - Got 1, expected 0
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/social/browser_social_workercrash.js | Exception thrown at resource://gre/modules/SocialService.jsm:405 - Error: SocialService.addProvider: provider with this origin already exists
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/toolkit/components/social/test/browser/browser_workerAPI.js | update of profile from our provider - Got https://example.com, expected http://example.com
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/toolkit/components/social/test/browser/browser_workerAPI.js | Test timed out
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/toolkit/mozapps/extensions/test/browser/browser_discovery.js | Name should be correct - Got Demo Social Service, expected provider 1
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/toolkit/mozapps/extensions/test/browser/browser_discovery.js | Version should be correct - Got 1, expected
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/toolkit/mozapps/extensions/test/browser/browser_discovery.js | Name should be correct - Got Demo Social Service, expected provider 1
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/toolkit/mozapps/extensions/test/browser/browser_discovery.js | Version should be correct - Got 1, expected
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/toolkit/mozapps/extensions/test/browser/browser_discovery.js | Name should be correct - Got Demo Social Service, expected provider 1
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/toolkit/mozapps/extensions/test/browser/browser_discovery.js | Version should be correct - Got 1, expected
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/toolkit/mozapps/extensions/test/browser-window/browser_discovery.js | Name should be correct - Got Demo Social Service, expected provider 1
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/toolkit/mozapps/extensions/test/browser-window/browser_discovery.js | Version should be correct - Got 1, expected
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/toolkit/mozapps/extensions/test/browser-window/browser_discovery.js | Name should be correct - Got Demo Social Service, expected provider 1
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/toolkit/mozapps/extensions/test/browser-window/browser_discovery.js | Version should be correct - Got 1, expected
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/toolkit/mozapps/extensions/test/browser-window/browser_discovery.js | Name should be correct - Got Demo Social Service, expected provider 1
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/toolkit/mozapps/extensions/test/browser-window/browser_discovery.js | Version should be correct - Got 1, expected

Comment 21

[Matthew N. [:MattN]]

(In reply to Manish Goregaokar [:manishearth] from comment #20)
I already starred (*) those two failures since they aren't related to this patch AFAICT.

I will look again in a few hours or tomorrow and push it if there are no failures.

Comment 22

[Manish Goregaokar [:manishearth]]

Ah. Was in the process of posting a second comment with a breakdown of the tests. Realized that there were still unfinished tests halfway through it.

Where would the test failures come from, though? Previous patches pushed to try?

IIRC last time we had similar issues, some yellow unrelated failures from XP.

Comment 23

[Matthew N. [:MattN]]

They are intermittent failures (often caused by tests that have race conditions). When you click on an orange run, you will see suggestions of possibly related intermittent failures and the two from comment 21 match known failures.

Comment 24

[Matthew N. [:MattN]]

Try looked good so I pushed it.

https://hg.mozilla.org/integration/fx-team/rev/f331b33c345d

Comment 25

[Ryan VanderMeulen [:RyanVM]]

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

Comment 26

[Neil Deakin]

This likely caused the intermittent orange bug 965534.