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

RESOLVED FIXED in Firefox 20

Status

()

defect
--
critical
RESOLVED FIXED
8 years ago
6 years ago

People

(Reporter: smaug, Assigned: mats)

Tracking

(Depends on 1 bug, 4 keywords)

unspecified
mozilla22
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(blocking-b2g:tef+, firefox19- wontfix, firefox20+ fixed, firefox21+ fixed, firefox22+ fixed, firefox-esr1720+ fixed, b2g1820+ fixed, b2g18-v1.0.0 wontfix, b2g18-v1.0.1 fixed)

Details

(Whiteboard: [sg:audit] [testcase in bug 817219][adv-main20+][adv-esr1705+])

Attachments

(7 attachments, 3 obsolete attachments)

...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().
Assignee

Comment 1

8 years ago
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
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
Assignee

Comment 3

8 years ago
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?
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.
(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?
> Or perhaps just check mCurrentEventContent->GetCurrentDoc() == mDocument

That's equivalent, yes.
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?
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.
But the patch breaks some chrome and browser chrome tests :/
Something to do with d&d
browser_dragdrop.js
Something like this should be very safe.
Assignee

Comment 12

8 years ago
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.
(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
Assignee

Comment 14

8 years ago
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.
if weak frames fixes the problem then guessing sg:critical
Whiteboard: [sg:critical?]
blocking2.0: --- → -
Assignee: nobody → Olli.Pettay
(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?
Assignee

Comment 17

8 years ago
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.
Assignee

Comment 18

8 years ago
Posted patch wip5Splinter Review
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)
Assignee

Comment 19

8 years ago
Posted patch wip6Splinter Review
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...
Assignee

Comment 20

8 years ago
Posted patch wip8Splinter Review
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
Assignee

Comment 21

8 years ago
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?
Assignee

Comment 22

8 years ago
Comment on attachment 525205 [details] [diff] [review]
wip8

Still green on Try, although it still hasn't completed all tests...
Attachment #525205 - Flags: review?(Olli.Pettay)
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.
Attachment #525205 - Flags: review?(Olli.Pettay) → review+
Assignee

Comment 24

8 years ago
> 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().
Assignee

Comment 25

8 years ago
Posted patch wip10 (obsolete) — Splinter Review
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/mpalmgren@mozilla.com-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!)
Assignee

Comment 26

8 years ago
Posted patch wip11Splinter Review
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.
Attachment #525815 - Attachment is obsolete: true
Attachment #525900 - Flags: review?(Olli.Pettay)
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!
Attachment #525900 - Flags: review?(Olli.Pettay) → review-
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?
Whiteboard: [sg:critical?] → [sg:critical?][needs answer to c28 from Mats or Olli]
Is this a regression in Firefox 4, or did it affect Firefox 3.6/3.5 as well?
Keywords: testcase-wanted
I believe this problem has been there all the way back from 1999.
(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)
blocking1.9.2: --- → needed
Whiteboard: [sg:critical?][needs answer to c28 from Mats or Olli] → [sg:critical?]
Whiteboard: [sg:critical?] → [sg:audit]

Comment 32

7 years ago
Is anyone planning to continue work on this?
Keywords: sec-audit
Oh, we certainly should fix this.
Mats,  could you update the patch?
Assignee

Comment 34

6 years ago
This fell off my radar, sorry.  I'm working on it again now.
(The crash test in bug 817219 demonstrates this bug.)
Assignee: bugs → matspal
Severity: normal → critical
Whiteboard: [sg:audit] → [sg:audit] [testcase in bug 817219]
Assignee

Updated

6 years ago
Assignee

Comment 36

6 years ago
Posted patch fix, v14 (obsolete) — Splinter Review
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
Attachment #706933 - Attachment is obsolete: true
Attachment #707057 - Flags: review?(bugs)
Assignee

Updated

6 years ago
Keywords: testcase-wanted
Hardware: x86_64 → All
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
Attachment #707057 - Flags: review?(bugs) → review+
Assignee

Comment 38

6 years ago
> ->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.
Assignee

Comment 40

6 years ago
Posted patch fix, v15Splinter Review
s/GetDocument/GetCurrentDoc/ per review comment, carrying over r+.
Attachment #707057 - Attachment is obsolete: true
Attachment #707122 - Flags: review+
Assignee

Comment 41

6 years ago
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.
Attachment #707122 - Flags: sec-approval?
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.
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.
This can land on 3/7 then. Giving it sec-approval+ with the caveat that it not land until then.
Attachment #707122 - Flags: sec-approval? → sec-approval+
Tracking for b2g18 branch, please nominate for uplift on that branch as well when this is landed to trunk.
Batch edit: Bugs marked status-b2g18: affected after 2/13 branching of v1.0.1 are now also status-b2g18-v1.0.1: affected
Please remember to nominate this for uplift with a risk evaluation by Monday so this gets into our next Beta if possible.
https://hg.mozilla.org/mozilla-central/rev/d5ec1f00ccd2
Status: NEW → RESOLVED
blocking1.9.2: needed → ---
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
blocking2.0: - → ---
Assignee

Comment 50

6 years ago
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
Attachment #707122 - Flags: approval-mozilla-esr17?
Attachment #707122 - Flags: approval-mozilla-beta?
Attachment #707122 - Flags: approval-mozilla-b2g18?
Attachment #707122 - Flags: approval-mozilla-aurora?
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.
Attachment #707122 - Flags: approval-mozilla-esr17?
Attachment #707122 - Flags: approval-mozilla-esr17+
Attachment #707122 - Flags: approval-mozilla-beta?
Attachment #707122 - Flags: approval-mozilla-beta+
Attachment #707122 - Flags: approval-mozilla-b2g18?
Attachment #707122 - Flags: approval-mozilla-b2g18+
Attachment #707122 - Flags: approval-mozilla-aurora?
Attachment #707122 - Flags: approval-mozilla-aurora+
Assignee

Comment 52

6 years ago
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...
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.
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
Assignee

Comment 57

6 years ago
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?
Flags: needinfo?(lsblakk)
I would argue that option #1 falls under a=test-only.
(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
Flags: needinfo?(lsblakk)
Assignee

Comment 60

6 years ago
#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)
Whiteboard: [sg:audit] [testcase in bug 817219] → [sg:audit] [testcase in bug 817219][adv-main20+][adv-esr1705+]
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.
blocking-b2g: --- → tef+
Depends on: 909006
Group: core-security
You need to log in before you can comment on or make changes to this bug.