635852 - Investigate if PresShell::GetCurrentEventFrame() should check that mCurrentEventContent's ownerDoc == mDocument

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Olli Pettay [:smaug][[e-mail]]]

...otherwise mCurrentEventFrame may start to point to an object in wrong
presshell if someone manages to move mCurrentEventContent to another document
and then call PresShell::GetCurrentEventFrame().

Comment 1

[Mats Palmgren (inactive)]

AFAIK, when a content node is moved to another document all its frames
are destroyed, and PresShell::NotifyDestroyingFrame nulls out
mCurrentEventFrame and mCurrentEventFrameStack if there's a match.
http://mxr.mozilla.org/mozilla-central/source/layout/base/nsPresShell.cpp#2988

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

What if mCurrentEventContent is set to an element which doesn't have frame
(so mCurrentEventFrame is null). Then the element is move to another document
and GetCurrentEventFrame() is called on the first presshell

Comment 3

[Mats Palmgren (inactive)]

Ah, I see what you mean now, mCurrentEventContent->GetPrimaryFrame()
could return its frame in the new pres shell.  Hmm, not good.

I think in the old days when GetPrimaryFrame was handled by
nsFrameManager it would have always returned null since there
would be no mapping anymore for the content in the old shell,
but now that GetPrimaryFrame() is just an accessor...

Maybe we should just null out mCurrentEventContent if it moves
to a new document?

Comment 4

[Boris Zbarsky [:bzbarsky]]

Alternately, we could add a check that the frame's presshell is the right one.  Certainly for 2.0 that seems like a safer change, though the other is more generally correct.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #4)
> Alternately, we could add a check that the frame's presshell is the right one. 
Or perhaps just check mCurrentEventContent->GetCurrentDoc() == mDocument
before setting mCurrentEventFrame?

Comment 6

[Boris Zbarsky [:bzbarsky]]

> Or perhaps just check mCurrentEventContent->GetCurrentDoc() == mDocument

That's equivalent, yes.

Comment 7

[Johnny Stenback (:jst)]

I'm not sure I understand how this is a security sensitive bug? Sounds to me like we may leak references to elements from a different origin, but those elements should not be accessable to JS etc (because of wrappers). Am I missing something here?

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

mCurrentEventContent isn't the problem (the possible DOM event is anyway 
dispatched to mCurrentEventContent, no matter in which presshell it is in), 
but having mCurrentEventFrame to point
to a frame in different presshell, since in that case mCurrentEventFrame doesn't 
get cleared when the frame is destroyed.
So this might lead to just some crashes, if we assume that pointer to
frames are safe.

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Attachment: something like this

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

But the patch breaks some chrome and browser chrome tests :/
Something to do with d&d
browser_dragdrop.js

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

Attachment: use weak frames

Something like this should be very safe.

Comment 12

[Mats Palmgren (inactive)]

You can get mCurrentEventContent through nsIPresShell::GetEventTargetContent.
The only consumer seems to be nsEventStateManager, but it has a
GetEventTargetContent too, and there are some consumers of that under
content/ and layout/ -- are those all safe?  There are several
content->GetPrimaryFrame() calls in ESM for example, do we need
to change those to mPresContext->GetPrimaryFrameFor(content) ?

Fwiw, I pushed a fix to Tryserver that just nulls out mCurrentEventContent
and in mCurrentEventContentStack.  It passed unit tests and it seems to
work fine locally on Linux.

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #12)
> Fwiw, I pushed a fix to Tryserver that just nulls out mCurrentEventContent
> and in mCurrentEventContentStack.
Nulls out when? That would be the right fix, assuming it nulls out also
the relevant nsCurrentEventFrame

Comment 14

[Mats Palmgren (inactive)]

In PresShell::NotifyDestroyingFrame() when we null out nsCurrentEventFrame.
This is what I pushed to Try:
http://hg.mozilla.org/try/rev/c22c42a0421c
I just checked a few (but not all) of the debug build logs -
the added assertions didn't trigger.

Comment 15

[Daniel Veditz [:dveditz]]

if weak frames fixes the problem then guessing sg:critical

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #14)
> In PresShell::NotifyDestroyingFrame() when we null out nsCurrentEventFrame.
But what if mCurrentEventFrame is null and mCurrentEventContent is in a different
document and PresShell::GetCurrentEventFrame() is called?

Comment 17

[Mats Palmgren (inactive)]

Yeah, good point.  I think what you suggested in comment 5 is enough.

PresShell::NotifyDestroyingFrame already nulls out mCurrentEventFrame
and entries in mCurrentEventFrameStack, so there is no risk
PopCurrentEventInfo() will restore a non-null mCurrentEventFrame
when the content has moved.

Comment 18

[Mats Palmgren (inactive)]

Attachment: wip5

This patch hangs mochitest --browser-chrome in browser_dragdrop.js:
http://tbpl.mozilla.org/?tree=MozillaTry&rev=f74353f9d70c
(it also hangs with just the second hunk)

Comment 19

[Mats Palmgren (inactive)]

Attachment: wip6

This fixed the hang in browser_dragdrop.js.
Possibly just a bug in nsDOMWindowUtils::DispatchDOMEventViaPresShell(),
it should dispatch the event to the event target's shell, right?

There might be more of that sort of thing in ESM...

Comment 20

[Mats Palmgren (inactive)]

Attachment: wip8

The change to DispatchDOMEventViaPresShell (wip6) made a chrome test fail:
7928 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/toolkit/content/tests/chrome/test_browser_drop.xul | text/x-moz-url drop on browser with cancelled drop event link - got "http://www.example.org", expected ""

http://mxr.mozilla.org/mozilla-central/source/toolkit/content/tests/chrome/window_browser_drop.xul#84
the failed test (line 62-65) does a synthesizeDrop() on a non-chrome <body>
that has an ondrop handler doing event.preventDefault() which is expected
to cancel the drop.

Previously that passed since DispatchDOMEventViaPresShell used the shell
associated with the nsDOMWindowUtils *chrome* window.
In the non-chrome case ESM will set dragSession->SetOnlyChromeDrop(true)
when handling the NS_DRAGDROP_OVER:
http://mxr.mozilla.org/mozilla-central/source/content/events/src/nsEventStateManager.cpp#3215
which prevents the ondrop handler from being called.

Adding ondragover='event.preventDefault()' to the test makes it pass,
so I'm guessing this is just a bug in the test.

It's green on Try so far...
http://tbpl.mozilla.org/?tree=MozillaTry&rev=95e21499e5be

Comment 21

[Mats Palmgren (inactive)]

If I add NS_ASSERTION(mCurrentEventContent->GetDocument(), "null doc");
in the else-part in GetCurrentEventFrame() it triggers from a couple of places.
Why are we dispatching events for content that's not in a document?
And why didn't PresShell::ContentRemoved catch that?

Comment 22

[Mats Palmgren (inactive)]

Comment on attachment 525205 [details] [diff] [review]
wip8

Still green on Try, although it still hasn't completed all tests...

Comment 23

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 525205 [details] [diff] [review]
wip8

>diff --git a/dom/base/nsDOMWindowUtils.cpp b/dom/base/nsDOMWindowUtils.cpp
>--- a/dom/base/nsDOMWindowUtils.cpp
>+++ b/dom/base/nsDOMWindowUtils.cpp
>@@ -1094,31 +1094,31 @@ nsDOMWindowUtils::DispatchDOMEventViaPre
>                                                nsIDOMEvent* aEvent,
>                                                PRBool aTrusted,
>                                                PRBool* aRetVal)
> {
>   if (!nsContentUtils::IsCallerTrustedForRead()) {
>     return NS_ERROR_DOM_SECURITY_ERR;
>   }
> 
>-  nsPresContext* presContext = GetPresContext();
>-  NS_ENSURE_STATE(presContext);
>-  nsCOMPtr<nsIPresShell> shell = presContext->GetPresShell();
>-  NS_ENSURE_STATE(shell);
>   nsCOMPtr<nsIPrivateDOMEvent> event = do_QueryInterface(aEvent);
>   NS_ENSURE_STATE(event);
>   event->SetTrusted(aTrusted);
>   nsEvent* internalEvent = event->GetInternalNSEvent();
>   NS_ENSURE_STATE(internalEvent);
>   nsCOMPtr<nsIContent> content = do_QueryInterface(aTarget);
>   NS_ENSURE_STATE(content);
>+  nsCOMPtr<nsIDocument> targetDoc = content->GetDocument();
>+  NS_ENSURE_STATE(targetDoc);
>+  nsRefPtr<nsIPresShell> targetShell = targetDoc->GetShell();
>+  NS_ENSURE_STATE(targetShell);
> 
>   nsEventStatus status = nsEventStatus_eIgnore;
>-  shell->HandleEventWithTarget(internalEvent, nsnull, content,
>-                               &status);
>+  targetShell->HandleEventWithTarget(internalEvent, nsnull, content,
>+                                     &status);
>   *aRetVal = (status != nsEventStatus_eConsumeNoDefault);
>   return NS_OK;
> }
Actually, could you change this so that we throw if 
aTarget's owner document's window isn't the same as 
mWindow.
You may need to tweak some testcase too.



 -5061,16 +5061,26 @@ PresShell::ContentRemoved(nsIDocument *a
>     mFrameConstructor->RestyleForRemove(aContainer->AsElement(), aChild,
>                                         oldNextSibling);
> 
>   PRBool didReconstruct;
>   mFrameConstructor->ContentRemoved(aContainer, aChild, oldNextSibling,
>                                     nsCSSFrameConstructor::REMOVE_CONTENT,
>                                     &didReconstruct);
> 
>+  if (aChild == mCurrentEventContent) {
>+    mCurrentEventContent = nsnull;
>+  }
>+  PRUint32 length = mCurrentEventContentStack.Count();
>+  for (PRUint32 i = 0; i < length; ++i) {
>+    if (aChild == mCurrentEventContentStack.ObjectAt(i)) {
>+      mCurrentEventContentStack.ReplaceObjectAt(nsnull, i);
>+    }
>+  }
What if mCurrentEventContent is a descendant of aChild?
Perhaps you could check here that if !mCurrentEventContent->IsInDoc()
remove mCurrentEventContent and iterate mCurrentEventContentStack.

Comment 24

[Mats Palmgren (inactive)]

> What if mCurrentEventContent is a descendant of aChild?

Good catch, maybe that's why I didn't catch the node that triggered
the "null doc" assertion I added above.  I'll look into it.

> Perhaps you could check here that if !mCurrentEventContent->IsInDoc()

Good idea, but IsInDoc() is true for aChild at this point.

It looks like this is the wrong place to hook into since
I'm sure we traverse the nodes in the removed sub-tree at
some point the clear the flag.  UnbindFromTree looks like
a better place...  or rather, after UnbindFromTree has been
done on the nodes... then a single call to null out those
CurrentEventContent nodes in PresShell that !IsInDoc().

Comment 25

[Mats Palmgren (inactive)]

Attachment: wip10

This patch passed unit tests
http://tbpl.mozilla.org/?tree=MozillaTry&rev=e9e0963ef28c

and there's only a single "null doc" assertion (haven't investigated it yet):
Win7 mochitests-5, toolkit/components/prompts/test/test_bug619644.html
http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-e9e0963ef28c/tryserver-win32-debug/tryserver_win7-debug_test-mochitests-5-build944.txt.gz

I added a "pres shell notification" (that cleans up !IsInDoc() nodes)
in nsINode::doRemoveChildAt after the UnbindFromTree() call.
(UnbindFromTree is what clears a bit and makes IsInDoc() false, AFAICT)

I added the "ownerDoc->GetWindow() == mWindow" test you asked for;
fallout for that:
toolkit/components/prompts/test/test_bug625187.html
testing/mochitest/tests/SimpleTest/EventUtils.js  (in synthesizeDrop!)

Comment 26

[Mats Palmgren (inactive)]

Attachment: wip11

Remove a couple of debug printfs and the "null doc" assertion.

I'm not sure if trying to catch content removal is the best
approach here.  Removing direct access to 'mCurrentEventContent'
using an internal accessor that nulls it out if its document
isn't the same as the shell's might be a simpler solution.

Comment 27

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 525900 [details] [diff] [review]
wip11

> nsINode::doRemoveChildAt(PRUint32 aIndex, PRBool aNotify,
>                          nsIContent* aKid, nsAttrAndChildArray& aChildArray,
>                          PRBool aMutationEvent)
> {
>   nsIDocument* doc = GetCurrentDoc();
>+  nsRefPtr<nsIPresShell> ps = doc ? doc->GetShell() : nsnull;

This adds two virtual method calls (AddRef and Release)
when the element is in a doc which has presshell.


>+  if (ps) {
>+    ps->NotifyUnbindFromTree();
>+  }
And this is yet another virtual call.

3 virtual calls is quite a bit in a reasonable hot code path.

As far as I see, ps can be just raw nsIPresShell* if the pointer is taken right before
it is used. And NotifyUnbindFromTree could be inline if mCurrentEventContent and mCurrentEventContentStack
was moved to nsIPresShell.
But I wonder if that is enough, performance-vise. Needs some profiling.
We could add a flag to node if presshell has a reference to it and call
NotifyUnbindFromTree only in that case. Or use the flag in PresShell::ContentRemoved
to clear mCurrentEventContent/mCurrentEventContentStack.


Sorry for the delay in reviewing!

Comment 28

[Johnny Stenback (:jst)]

There's a bunch of reviewed wip patches, can any of them land, or are they not complete fixes, or are we working towards an even better fix here?

Comment 29

[Daniel Veditz [:dveditz]]

Is this a regression in Firefox 4, or did it affect Firefox 3.6/3.5 as well?

Comment 30

[Olli Pettay [:smaug][[e-mail]]]

I believe this problem has been there all the way back from 1999.

Comment 31

[Johnathan Nightingale [:johnath]]

(In reply to comment #30)
> I believe this problem has been there all the way back from 1999.

Based on this, clearing the tracking flag - johnny, re-nom if you think we still need to track this against FF5 specifically (as opposed to just ASAP)

Comment 32

[Josh Aas]

Is anyone planning to continue work on this?

Comment 33

[Olli Pettay [:smaug][[e-mail]]]

Oh, we certainly should fix this.
Mats,  could you update the patch?

Comment 34

[Mats Palmgren (inactive)]

This fell off my radar, sorry.  I'm working on it again now.
(The crash test in bug 817219 demonstrates this bug.)

Comment 35

[Mats Palmgren (inactive)]

Attachment: fix, v13

https://tbpl.mozilla.org/?usebuildbot=1&tree=Try&rev=a3ea66a9dccf

Comment 36

[Mats Palmgren (inactive)]

Attachment: fix, v14

Added PresShell::GetCurrentEventContent() that checks the document of
mCurrentEventContent each time it's needed.  I think this is a less hot
path than notifying each UnbindFromTree.

https://tbpl.mozilla.org/?tree=Try&rev=9a3331266c11

Comment 37

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 707057 [details] [diff] [review]
fix, v14


>+PresShell::GetCurrentEventContent()
>+{
>+  if (mCurrentEventContent &&
>+      mCurrentEventContent->GetDocument() != mDocument) {
->GetCurrentDoc()
.GetDocument is old and should go away. Same also elsewhere.



> <browser id="contentchild" type="content" width="100" height="100"
>-         src="data:text/html,&lt;html draggable='true'&gt;&lt;body draggable='true' style='width: 100px; height: 100px;' ondrop='if (window.stopMode) event.stopPropagation(); if (window.cancelMode) event.preventDefault();'&gt;&lt;/body&gt;&lt;/html&gt;"/>
>+         src="data:text/html,&lt;html draggable='true'&gt;&lt;body draggable='true' style='width: 100px; height: 100px;' ondragover='event.preventDefault()' ondrop='if (window.stopMode) event.stopPropagation(); if (window.cancelMode) event.preventDefault();'&gt;&lt;/body&gt;&lt;/html&gt;"/>
Could you explain this change? Feels odd if dragover listener is suddenly needed in this testcase. 
This explained, r=me

Comment 38

[Mats Palmgren (inactive)]

> ->GetCurrentDoc()

OK, will fix.

> Could you explain this change?

This is explained in comment 20.  I still think it's just a bug in this test.

Comment 39

[Olli Pettay [:smaug][[e-mail]]]

k

Comment 40

[Mats Palmgren (inactive)]

Attachment: fix, v15

s/GetDocument/GetCurrentDoc/ per review comment, carrying over r+.

Comment 41

[Mats Palmgren (inactive)]

Comment on attachment 707122 [details] [diff] [review]
fix, v15

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
Don't know.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
It reveals that it has something to do with DOM events, and that we're
tweaking some document checks.  The (existing) comment mentions removing
content.  It might be used as guidance to find a crash test.

Which older supported branches are affected by this flaw?
All.

If not all supported branches, which bug introduced the flaw?
N/A.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
I expect the same patch will apply, with minor modifications.

How likely is this patch to cause regressions; how much testing does it need?
Medium-to-low risk.  I think our regression tests covers this code
pretty well.

Comment 42

[Al Billings [:abillings - ex-MoCo]]

Adding Release Management so we can discuss sec-approval. This is a sec-critical that affects everything so we need to decide when we want to take it and where.

Comment 43

[Lukas Blakk [:lsblakk] use ?needinfo]

Given that this has been around for a long time we shouldn't try to cram it last-minute into FF19, let's track for FF20 as the first release to ship with the fix and therefore leave sec-approval for another 5 weeks, landing this everywhere in time for FF20 beta 4.  I'll leave b2g18 nommed since we'll have to evaluate at that time where this will need to land for b2g.

Comment 44

[Al Billings [:abillings - ex-MoCo]]

This can land on 3/7 then. Giving it sec-approval+ with the caveat that it not land until then.

Comment 45

[Lukas Blakk [:lsblakk] use ?needinfo]

Tracking for b2g18 branch, please nominate for uplift on that branch as well when this is landed to trunk.

Comment 46

[Lukas Blakk [:lsblakk] use ?needinfo]

Batch edit: Bugs marked status-b2g18: affected after 2/13 branching of v1.0.1 are now also status-b2g18-v1.0.1: affected

Comment 47

[Mats Palmgren (inactive)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d5ec1f00ccd2

Comment 48

[Lukas Blakk [:lsblakk] use ?needinfo]

Please remember to nominate this for uplift with a risk evaluation by Monday so this gets into our next Beta if possible.

Comment 49

[Ryan VanderMeulen [:RyanVM]]

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

Comment 50

[Mats Palmgren (inactive)]

Comment on attachment 707122 [details] [diff] [review]
fix, v15

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: sec-critical crash
Testing completed (on m-c, etc.): 2 days on m-c
Risk to taking this patch (and alternatives if risky): Medium-to-low risk, see comment 41
String or UUID changes made by this patch: none

Comment 51

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 707122 [details] [diff] [review]
fix, v15

Approving for uplift - please get this on branches today so it's in tomorrow morning's beta build.

Comment 52

[Mats Palmgren (inactive)]

The mochitest/tests/SimpleTest/ChromeUtils.js change to synthesizeDrop() doesn't
apply for mozilla-esr17 and mozilla-b2g18, because its last arg "aDestWindow"
was added later in bug 816406.  I suspect that this change isn't needed on
these branches since there can't be any tests that use that arg.  So I'll land
without this change and see what breaks.  I might need to tweak some chrome
mochitest after landing though...

Comment 53

[Mats Palmgren (inactive)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/a11d11b69ee7
https://hg.mozilla.org/releases/mozilla-beta/rev/12f0f87e4497

Comment 54

[Mats Palmgren (inactive)]

without the ChromeUtils.js change:
https://hg.mozilla.org/releases/mozilla-esr17/rev/8e59b75b4bb9
https://hg.mozilla.org/releases/mozilla-b2g18/rev/6f2bd6ce1556

Comment 55

[Ryan VanderMeulen [:RyanVM]]

Backed out on esr17 and b2g18 for mochitest-other failures.
https://hg.mozilla.org/releases/mozilla-b2g18/rev/628bc2b9cd4a
https://hg.mozilla.org/releases/mozilla-esr17/rev/2a738cf8eb6e

https://tbpl.mozilla.org/php/getParsedLog.php?id=20579066&tree=Mozilla-B2g18

14890 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/toolkit/content/tests/chrome/test_browser_drop.xul | uncaught exception - HierarchyRequestError: Node cannot be inserted at the specified point in the hierarchy at chrome://mochikit/content/tests/SimpleTest/ChromeUtils.js:259
14891 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/toolkit/content/tests/chrome/test_browser_drop.xul | Test timed out.

Comment 56

[Ryan VanderMeulen [:RyanVM]]

mochitest-bc failures too:
https://tbpl.mozilla.org/php/getParsedLog.php?id=20579349&tree=Mozilla-Esr17

TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/toolkit/mozapps/extensions/test/browser/browser_dragdrop.js | uncaught exception - HierarchyRequestError: Node cannot be inserted at the specified point in the hierarchy at chrome://mochikit/content/tests/SimpleTest/ChromeUtils.js:259
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/toolkit/mozapps/extensions/test/browser/browser_dragdrop.js | Test timed out
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/toolkit/mozapps/extensions/test/browser/browser_dragdrop.js | Found a tab after previous test timed out: about:addons
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/toolkit/mozapps/extensions/test/browser-window/browser_dragdrop.js | uncaught exception - HierarchyRequestError: Node cannot be inserted at the specified point in the hierarchy at chrome://mochikit/content/tests/SimpleTest/ChromeUtils.js:259
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/toolkit/mozapps/extensions/test/browser-window/browser_dragdrop.js | Test timed out
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/toolkit/mozapps/extensions/test/browser-window/browser_dragdrop.js | Found unexpected add-ons manager window still open

Comment 57

[Mats Palmgren (inactive)]

We need the ChromeUtils.js changes from bug 816406 to fix the failing
tests on esr17/b2g18.  So the alternatives are:

1. import this change into esr17/b2g18 and then try landing the original patch again:
https://hg.mozilla.org/mozilla-central/diff/d1fba48c7253/testing/mochitest/tests/SimpleTest/ChromeUtils.js
(it appears to fix at least one failing test locally)

2. disable the failing tests

3. wontfix this bug for esr17/b2g18

I think 1 is the best option at this point, do I have approval on the additional change?

Comment 58

[Ryan VanderMeulen [:RyanVM]]

I would argue that option #1 falls under a=test-only.

Comment 59

[Alex Keybl [:akeybl]]

(In reply to Mats Palmgren [:mats] from comment #57)
> We need the ChromeUtils.js changes from bug 816406 to fix the failing
> tests on esr17/b2g18.  So the alternatives are:
> 
> 1. import this change into esr17/b2g18 and then try landing the original
> patch again:
> https://hg.mozilla.org/mozilla-central/diff/d1fba48c7253/testing/mochitest/
> tests/SimpleTest/ChromeUtils.js
> (it appears to fix at least one failing test locally)
> 
> 2. disable the failing tests
> 
> 3. wontfix this bug for esr17/b2g18
> 
> I think 1 is the best option at this point, do I have approval on the
> additional change?

If there isn't a #4 where we come up with a workaround, and #1 is low risk, #1 is the way to go. a=akeybl

Comment 60

[Mats Palmgren (inactive)]

#1 is low risk, the additional patch from bug 816406 only affects mochitests
as far as I know.

https://hg.mozilla.org/releases/mozilla-esr17/rev/ddb2d0ed454b
https://hg.mozilla.org/releases/mozilla-esr17/rev/be08f5283f46

(the Mozilla-B2g18 tree is still closed due to bug 850631)

Comment 61

[Mats Palmgren (inactive)]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/5cd66d870483
https://hg.mozilla.org/releases/mozilla-b2g18/rev/dc3add076016

Comment 62

[Alex Keybl [:akeybl]]

Since Firefox 20 is now released, v1.0.1 branches are still open, and this is considered low risk, we can uplift to v1.0.1. Marking as tef+ and flipping status-b2g18-v1.0.1 to affected.

Comment 63

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/15a89537eca5
https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/27b5f064d180