xul.dll!mozilla::MediaFormatReader::InitLayersBackendType() [MediaFormatReader.cpp:c44c01dfd264 : 518 + 0x0]

RESOLVED FIXED in Firefox -esr45

Status

()

P1
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: cbook, Assigned: jwwang)

Tracking

(Blocks: 1 bug, 5 keywords)

45 Branch
mozilla53
assertion, crash, csectype-uaf, regression, sec-high
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox49 wontfix, firefox-esr4550+ fixed, firefox50+ fixed, firefox51+ fixed, firefox52+ fixed, firefox53- fixed)

Details

(Whiteboard: [adv-main50.1+][adv-esr45.6+], URL)

Attachments

(6 attachments, 2 obsolete attachments)

(Reporter)

Description

2 years ago
found via bughunter and reproduced on latest trunk nightly windows build.

Steps to reproduced: 
--> Load https://agile-spire-1086.herokuapp.com/
---> Crash on load on a new profile

filing as security sensitive bug because of Crash Adress 0xffffffffe5e5e5e5

Crashes opt and debug builds nightly only

Assertion failure: mShutdown, at c:/builds/moz2_slave/m-cen-w32-d-00000000000000
0000/build/src/dom/media/MediaDecoderReader.cpp:109
[Child 3352] WARNING: MediaFormatReader without a decoder owner, can't get HWAcc
el: file c:/builds/moz2_slave/m-cen-w32-d-000000000000000000/build/src/dom/media
/MediaFormatReader.cpp, line 520
[Child 3352] WARNING: MediaFormatReader without a decoder owner, can't get HWAcc
el: file c:/builds/moz2_slave/m-cen-w32-d-000000000000000000/build/src/dom/media
/MediaFormatReader.cpp, line 520
#01: mozilla_dump_image[C:\bughunter\firefox-52.0a1.en-US.win32\firefox\xul.dll
+0x17fb96e]
#02: mozilla_dump_image[C:\bughunter\firefox-52.0a1.en-US.win32\firefox\xul.dll
+0x183339c]
#03: NS_StringSetIsVoid[C:\bughunter\firefox-52.0a1.en-US.win32\firefox\xul.dll
+0x20e4f0]
#04: XRE_AddStaticComponent[C:\bughunter\firefox-52.0a1.en-US.win32\firefox\xul.
dll +0x1e7c49]
#05: XRE_AddStaticComponent[C:\bughunter\firefox-52.0a1.en-US.win32\firefox\xul.
dll +0x1ef90e]
#06: XRE_AddStaticComponent[C:\bughunter\firefox-52.0a1.en-US.win32\firefox\xul.
dll +0x1e5982]
#07: NS_StringSetIsVoid[C:\bughunter\firefox-52.0a1.en-US.win32\firefox\xul.dll
+0x20de89]
#08: mozilla::net::LoadInfo::TriggeringPrincipal[C:\bughunter\firefox-52.0a1.en-
US.win32\firefox\xul.dll +0x5b28b7]
#09: mozilla::net::LoadInfo::TriggeringPrincipal[C:\bughunter\firefox-52.0a1.en-
US.win32\firefox\xul.dll +0x593735]
#10: mozilla::net::LoadInfo::TriggeringPrincipal[C:\bughunter\firefox-52.0a1.en-
US.win32\firefox\xul.dll +0x5936ed]
#11: mozilla::net::LoadInfo::TriggeringPrincipal[C:\bughunter\firefox-52.0a1.en-
US.win32\firefox\xul.dll +0x593436]
#12: XRE_AddStaticComponent[C:\bughunter\firefox-52.0a1.en-US.win32\firefox\xul.
dll +0x1ea3d3]
#13: PR_NativeCreateThread[C:\bughunter\firefox-52.0a1.en-US.win32\firefox\nss3.
dll +0x183520]
#14: PR_MD_WAIT_CV[C:\bughunter\firefox-52.0a1.en-US.win32\firefox\nss3.dll +0x1
77671]
#15: o__CIpow[C:\bughunter\firefox-52.0a1.en-US.win32\firefox\ucrtbase.DLL +0x3d
5ef]
#16: BaseThreadInitThunk[C:\Windows\system32\kernel32.dll +0x4ee1c]
#17: RtlInitializeExceptionChain[C:\Windows\SYSTEM32\ntdll.dll +0x637eb]
#18: RtlInitializeExceptionChain[C:\Windows\SYSTEM32\ntdll.dll +0x637be]
[Child 3352] WARNING: MediaFormatReader without a decoder owner, can't get HWAcc
el: file c:/builds/moz2_slave/m-cen-w32-d-000000000000000000/build/src/dom/media
/MediaFormatReader.cpp, line 520
[Child 3352] WARNING: MediaFormatReader without a decoder owner, can't get HWAcc
el: file c:/builds/moz2_slave/m-cen-w32-d-000000000000000000/build/src/dom/media
/MediaFormatReader.cpp, line 520
[Child 3352] WARNING: MediaFormatReader without a decoder owner, can't get HWAcc
el: file c:/builds/moz2_slave/m-cen-w32-d-000000000000000000/build/src/dom/media
/MediaFormatReader.cpp, line 520
[Child 3352] WARNING: MediaFormatReader without a decoder owner, can't get HWAcc
el: file c:/builds/moz2_slave/m-cen-w32-d-000000000000000000/build/src/dom/media
/MediaFormatReader.cpp, line 520
Assertion failure: mShutdown, at c:/builds/moz2_slave/m-cen-w32-d-00000000000000
0000/build/src/dom/media/MediaDecoderReader.cpp:109
#01: mozilla_dump_image[C:\bughunter\firefox-52.0a1.en-US.win32\firefox\xul.dll
+0x17fb96e]
#02: mozilla_dump_image[C:\bughunter\firefox-52.0a1.en-US.win32\firefox\xul.dll
+0x183339c]
[Child 3352] WARNING: MediaFormatReader without a decoder owner, can't get HWAcc
el: file c:/builds/moz2_slave/m-cen-w32-d-000000000000000000/build/src/dom/media
/MediaFormatReader.cpp, line 520
#03: NS_StringSetIsVoid[C:\bughunter\firefox-52.0a1.en-US.win32\firefox\xul.dll
+0x20e4f0]
#04: XRE_AddStaticComponent[C:\bughunter\firefox-52.0a1.en-US.win32\firefox\xul.
dll +0x1e7c49]
#05: XRE_AddStaticComponent[C:\bughunter\firefox-52.0a1.en-US.win32\firefox\xul.
dll +0x1ef90e]
#06: XRE_AddStaticComponent[C:\bughunter\firefox-52.0a1.en-US.win32\firefox\xul.
dll +0x1e5982]
#07: NS_StringSetIsVoid[C:\bughunter\firefox-52.0a1.en-US.win32\firefox\xul.dll
+0x20de89]
#08: mozilla::net::LoadInfo::TriggeringPrincipal[C:\bughunter\firefox-52.0a1.en-
US.win32\firefox\xul.dll +0x5b28b7]
#09: mozilla::net::LoadInfo::TriggeringPrincipal[C:\bughunter\firefox-52.0a1.en-
US.win32\firefox\xul.dll +0x593735]
#10: mozilla::net::LoadInfo::TriggeringPrincipal[C:\bughunter\firefox-52.0a1.en-
US.win32\firefox\xul.dll +0x5936ed]
#11: mozilla::net::LoadInfo::TriggeringPrincipal[C:\bughunter\firefox-52.0a1.en-
US.win32\firefox\xul.dll +0x593436]
#12: XRE_AddStaticComponent[C:\bughunter\firefox-52.0a1.en-US.win32\firefox\xul.
dll +0x1ea3d3]
#13: PR_NativeCreateThread[C:\bughunter\firefox-52.0a1.en-US.win32\firefox\nss3.
dll +0x183520]
#14: PR_MD_WAIT_CV[C:\bughunter\firefox-52.0a1.en-US.win32\firefox\nss3.dll +0x1
77671]
#15: o__CIpow[C:\bughunter\firefox-52.0a1.en-US.win32\firefox\ucrtbase.DLL +0x3d
5ef]
#16: BaseThreadInitThunk[C:\Windows\system32\kernel32.dll +0x4ee1c]
#17: RtlInitializeExceptionChain[C:\Windows\SYSTEM32\ntdll.dll +0x637eb]
#18: RtlInitializeExceptionChain[C:\Windows\SYSTEM32\ntdll.dll +0x637be]
(Reporter)

Comment 1

2 years ago
Created attachment 8808097 [details]
bughunter stack

stack
(Reporter)

Comment 2

2 years ago
[Tracking Requested - why for this release]:

opt crash is https://crash-stats.mozilla.com/report/index/ca8a56de-9b6e-46ae-8770-c5c122161107 but i guess the report there is not so useful
tracking-firefox52: --- → ?
(Reporter)

Comment 3

2 years ago
also addional steps to reproduce:
-> when loaded i got a adblock information or so - you need to dismiss this
-> it might take 30 second to a minute to crash on the page
(Reporter)

Comment 4

2 years ago
Created attachment 8808106 [details]
another stack this time crash in  0  xul.dll!mozilla::BufferDecoder::BeginDecoding(mozilla::TaskQueue *) [BufferDecoder.cpp:c44c01dfd264 : 35 + 0xe]
(Reporter)

Updated

2 years ago
Blocks: 532972
Will look at it tomorrow.
Component: Audio/Video: Playback → Web Audio
Assignee: nobody → jyavenard
Blocks: 1312337
(Assignee)

Comment 6

2 years ago
Created attachment 8808681 [details] [diff] [review]
1315631_fix.patch
Assignee: jyavenard → jwwang
Status: NEW → ASSIGNED
Attachment #8808681 - Flags: review?(jyavenard)
(Assignee)

Updated

2 years ago
Component: Web Audio → Audio/Video: Playback
(Assignee)

Updated

2 years ago
No longer blocks: 1312337
Comment on attachment 8808681 [details] [diff] [review]
1315631_fix.patch

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

Awesome find !

r=me with DoInit being renamed to InitInternal

It's now the 2nd time we've had bugs for the same underlying reason. I wonder if there are any other places where tasks are dispatched in the constructor.
I also wonder if that could explain some super weird (and rare) crashes in the TrackBuffersManager / MediaSourceDemuxer destructor that makes no sense at all...

::: dom/media/MediaDecoderReader.h
@@ +387,5 @@
>    // Notify if this media is not seekable.
>    MediaEventProducer<void> mOnMediaNotSeekable;
>  
>  private:
> +  virtual nsresult DoInit() { return NS_OK; }

I'd prefer the name InitInternal, to keep consistency with other methods.
Attachment #8808681 - Flags: review?(jyavenard) → review+
[Tracking Requested - why for this release]: A gutfeel tells me that this is likely the reason for many other wierd crashes we've had for a while...
status-firefox50: --- → affected
status-firefox51: --- → affected
tracking-firefox50: --- → ?
Group: media-core-security
Ehsan, as you're the champion of static analysis stuff.

Would it be possible to have a static analyser that reports every constructors that take a refcount on "this" ?

This is not the first time this problem has bitten us, would like to make sure it never happens again.
Flags: needinfo?(ehsan)
Group: media-core-security
(Assignee)

Comment 10

2 years ago
Created attachment 8808892 [details] [diff] [review]
1315631_fix_v2.patch
Attachment #8808681 - Attachment is obsolete: true
Attachment #8808892 - Flags: review+
(Assignee)

Updated

2 years ago
Keywords: checkin-needed
Priority: -- → P1
this need a security rate first
Keywords: checkin-needed
Comment on attachment 8808892 [details] [diff] [review]
1315631_fix_v2.patch

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

::: dom/media/ogg/OggReader.cpp
@@ +164,5 @@
>      AbstractThread::MainThread()->Dispatch(task.forget());
>    }
>  }
>  
> +nsresult OggReader::InitInternal() {

could fix the style while at it there :)
(Assignee)

Comment 13

2 years ago
dom/media/ogg/OggReader.cpp is gonna be removed soon IIUC...
Comment on attachment 8808892 [details] [diff] [review]
1315631_fix_v2.patch

[Security approval request comment]
How easily could an exploit be constructed based on the patch? don't think it can be... it's a race where if a task dispatched in the constructor completes prior the constructor finishing

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? no

Which older supported branches are affected by this flaw? 45

If not all supported branches, which bug introduced the flaw? 1172264

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? easy to create, overall it's a one-liner

How likely is this patch to cause regressions; how much testing does it need? can't think of any
Attachment #8808892 - Flags: sec-approval?
Blocks: 1172264
(Reporter)

Comment 15

2 years ago
from irc:

11:53 <jya> That one is pretty nasty... I hope it'll get to 50
11:53 <jya> Can you make sure sec people review that asap?

Sylvestre, Ritu: fyi
Flags: needinfo?(sledru)
Flags: needinfo?(rkothari)
Flags: needinfo?(dveditz)
Flags: needinfo?(abillings)
Would be nice if we could get a sec rating on this.
Group: core-security → media-core-security
status-firefox49: --- → wontfix
status-firefox52: --- → affected
status-firefox-esr45: --- → affected
tracking-firefox51: --- → ?
tracking-firefox-esr45: --- → ?
Keywords: regression
Version: unspecified → 45 Branch
(Reporter)

Comment 17

2 years ago
also maybe related to bug 1257921
Could be sec-critical, if we believe someone could cause a reallocation with a different object
Keywords: csectype-uaf, sec-high
This can go in on 11/29, two weeks into the new cycle after we ship 50 this next week. So, sec-approval+ for checkin on 11/29 or later (not before).
Flags: needinfo?(abillings)
Whiteboard: [checkin on 11/29]
Attachment #8808892 - Flags: sec-approval? → sec-approval+
BTW, asking to ship a bug in 50 when we've already made the release builds and are about to ship? :-) Probably not going to happen. There is a 50.1 in December though that is possible.
Why such delay imposed?

Do we like crashes that much?
How do you suggest shipping it? We've already made 50 and are about to ship it. 51 isn't until January unless this gets into 50.1 (which you or others would need to make a case for). As soon as you check the code in, the fix is public, a potential zero day. This is ENTIRELY the reason why sec-approval exists: to gate checkins to limit vulnerability exposure. 

I'm saying that two weeks into the next release cycle, you can check in. I do this all the time (sometimes it is three or four for dangerous bugs that have very safe patches). 

So, yes, we love crashes. Thank you for asking.
> This is ENTIRELY the reason why sec-approval exists: to gate checkins to limit vulnerability exposure. 

as I mentioned in the sec? bit, I don't see how that could be exploitable. 
so how can not fixing this bug limit the vulnerability exposure? (rhetorical question)

Personally, I wouldn't have classified this bug as sec-high.
The bug 1259473 and its fix is identical. I guess a bad example, seeing that this was committed without approval :)
Comment on attachment 8808892 [details] [diff] [review]
1315631_fix_v2.patch

Approval Request Comment
[Feature/regressing bug #]: 1172264
[User impact if declined]: Crashes. We've had lots of unexplained crashes logged, fairly certain they are all due to this one.
[Describe test coverage new/current, TreeHerder]: manual test. can't run try yet.
[Risks and why]: None, it's same problem (and fix) in nature as bug 1259473). It's a very safe fix.
[String/UUID change made/needed]: None
Attachment #8808892 - Flags: approval-mozilla-release?
Attachment #8808892 - Flags: approval-mozilla-beta?
Attachment #8808892 - Flags: approval-mozilla-aurora?
We have already shipped Release Candidate build-2 to the beta channel for final sanity testing. If we have a "chemspill" type bug we can have a chemspill-type response and get a new build before the official release date, but it will be a chemspill type response and all the associated potential for burnout this imposes on the release and QA teams.

So far as we know we found this internally and aren't going to tip anyone off about it unless we check it in. Would be nice to do that _after_ this release than before.

(In reply to Jean-Yves Avenard [:jya] from comment #23)
> as I mentioned in the sec? bit, I don't see how that could be exploitable. 
> so how can not fixing this bug limit the vulnerability exposure? (rhetorical
> question)

That's a different question. The fact that Carsten could file this from a real-world crash would seem to indicate it could indeed be exploitable. Races are not unexploitable -- easy enough for attackers to try again and again until they win the race (unless losing means crashing safely, so they don't get to try more than once).
Flags: needinfo?(dveditz)
(Reporter)

Comment 26

2 years ago
from talking to jya i got the impression that this fix will some crashes we were already chasing in the past but had no attack point how to fix before we had this bug+patch here.

What about waiting the 2 weeks that Al set for checkin + asap uplift (after this landed on m-c) and then checking the crash-stats if the fix has a impact and if we see the impact in the crashes(or better crash signatures) requesting that this bug here got included if/when we do a point release for 50.0 like a 50.1
Comment 20 already mentioned 50.1 being a possibility.
Yes, let's do it in 50.1. This is an acceptable alternative I think.

Jean-Yves, I think we want this in esr too.
Flags: needinfo?(sledru)
tracking-firefox50: ? → +
tracking-firefox51: ? → +
tracking-firefox52: ? → +

Comment 29

2 years ago
(In reply to Jean-Yves Avenard [:jya] from comment #9)
> Ehsan, as you're the champion of static analysis stuff.
> 
> Would it be possible to have a static analyser that reports every
> constructors that take a refcount on "this" ?
> 
> This is not the first time this problem has bitten us, would like to make
> sure it never happens again.

Having a general analysis which detects whether a function that |this| is passed to ends up calling AddRef on it is difficult, since our current analyses mostly look at the AST structure of the program, and don't necessarily look into the callee functions' bodies.

However, having an analysis that flags this passed to things such as NewRunnableMethod (and other similar functions) is quite doable, and I think would be a good compromise.

Do you think that would be useful?  Can you please provide some examples of the other bugs where we've had this pattern?

Last but not least, thanks for flagging this!  It's awesome to try to find ways in which we prevent classes of security bugs by adding static analysis checks!  :-)
Flags: needinfo?(ehsan) → needinfo?(jyavenard)
Just like Sylvestre said.
Flags: needinfo?(rkothari)
(In reply to :Ehsan Akhgari from comment #29)
> Having a general analysis which detects whether a function that |this| is
> passed to ends up calling AddRef on it is difficult, since our current
> analyses mostly look at the AST structure of the program, and don't
> necessarily look into the callee functions' bodies.

:kentuckyfriedtakahe mentioned something that may be simpler to check that the refcount isn't 0 everywhere a new thread involved.

> 
> However, having an analysis that flags this passed to things such as
> NewRunnableMethod (and other similar functions) is quite doable, and I think
> would be a good compromise.
> 
> Do you think that would be useful?  Can you please provide some examples of
> the other bugs where we've had this pattern?

bug 1259473.

This one was a bit similar, in the exception that it was the failure to queue the task that caused the ref count to be dropped.

E.g. in the constructor you had in effect:
RefPtr<T> self = this;

That too should be forbidden.
(https://bugzilla.mozilla.org/show_bug.cgi?id=1259473#c19 I guess I should have acted on my gutfeel back then :( )

> 
> Last but not least, thanks for flagging this!  It's awesome to try to find
> ways in which we prevent classes of security bugs by adding static analysis
> checks!  :-)

those are bugs very difficult to determine from a stack trace, so I'm just being very selfish attempting to make our job easier.
Flags: needinfo?(jyavenard)
See Also: → bug 1259473
:ehsan should I create a new bug for that particular task?
Flags: needinfo?(ehsan)

Comment 33

2 years ago
Thanks Jean-Yves.  I filed a follow-up for the static analysis myself (bug 1317773), together with a proposed plan.  I'm planning to work on that bug...
Flags: needinfo?(ehsan)

Comment 34

2 years ago
I wrote a static analysis for detecting this pattern of bugs at compile time.  The analysis flagged the code being fixed here, but it *also* found this:

/home/ehsan/moz/src/dom/media/MediaDecoderReader.cpp:86:19: warning: `this' pointer cannot be passed as argument 1 to 'Connect<mozilla::MediaDecoderReader, void (mozilla::MediaDecoderReader::*)()>' of type 'mozilla::MediaDecoderReader *'

That is, passing |this| to Connect() here <http://searchfox.org/mozilla-central/rev/b4e6fa3527436bdbcd9dd2e1982382fe423431f3/dom/media/MediaDecoderReader.cpp#85> is affected by the same underlying issue.  For now, Connect() isn't actually grabbing a new reference to aThis but as soon as that changes, we have another potential security bug on our hands.

JW, since you were already touching this code, do you mind moving this part into Init() as well?  Thank you!
Flags: needinfo?(jwwang)
I had looked into this one as it too had grabbed my attention. 

However, this can't dispatch a task while we're in the current constructor. It's merely setting up the connection that will be called once the MediaEvent::Notify is later called.  

So as it is; it's safe.
(Assignee)

Comment 36

2 years ago
I will update the patch to avoid confusion. The general rule should be "don't pass this somewhere else in the constructor" and "add an annotation for the analysis to be happy if you are sure it is safe to pass this".
Flags: needinfo?(jwwang)

Comment 37

2 years ago
Yes, agreed the code is safe as is, but as I described, if Connect() is changed in the future to create and destroy a RefPtr on its argument or some such then this becomes a pattern for a new bug.
(In reply to :Ehsan Akhgari from comment #37)
> Yes, agreed the code is safe as is, but as I described, if Connect() is
> changed in the future to create and destroy a RefPtr on its argument or some
> such then this becomes a pattern for a new bug.

The issue here is a thread-safety issue. 

The MediaEvent is used via the main thread. And as such should be initialised on the main thread. 
Unit is called on the reader's taskqueue. 

There's the potential where it could be accessed on the main thread while being initialised on the reader's one
(Assignee)

Comment 39

2 years ago
Created attachment 8814522 [details] [diff] [review]
1315631_fix_v3.patch

Address comment 34.
Please land this patch instead of 1315631_fix_v2.patch.
Attachment #8814522 - Flags: review+
Can you also request uplift to ESR? Thanks.
Flags: needinfo?(jwwang)
jya, that needinfo in comment 40 might be for you.
Flags: needinfo?(jyavenard)
(Assignee)

Comment 42

2 years ago
(In reply to JW Wang [:jwwang] [:jw_wang] from comment #39)
> Created attachment 8814522 [details] [diff] [review]
> 1315631_fix_v3.patch
> 
> Address comment 34.
> Please land this patch instead of 1315631_fix_v2.patch.

1315631_fix_v3.patch is for Central only.
Comment on attachment 8808892 [details] [diff] [review]
1315631_fix_v2.patch

Sec-high, approved for all branches.
Attachment #8808892 - Flags: approval-mozilla-release?
Attachment #8808892 - Flags: approval-mozilla-release+
Attachment #8808892 - Flags: approval-mozilla-beta?
Attachment #8808892 - Flags: approval-mozilla-beta+
Attachment #8808892 - Flags: approval-mozilla-aurora?
Attachment #8808892 - Flags: approval-mozilla-aurora+

Comment 45

2 years ago
uplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/8402aa196fd69d6a978da9b8f4cc1b95c847db9e
status-firefox52: affected → fixed
status-firefox53: --- → affected
tracking-firefox53: --- → ?
JW is working on a rebased patch for Beta.
(Assignee)

Comment 47

2 years ago
Created attachment 8815594 [details] [diff] [review]
1315631_fix_bata.patch

Approval Request Comment
[Feature/Bug causing the regression]:1172264
[User impact if declined]:Crashes. We've had lots of unexplained crashes logged, fairly certain they are all due to this one.
[Is this code covered by automated tests?]:No.
[Has the fix been verified in Nightly?]:Yes.
[Needs manual test from QE? If yes, steps to reproduce]: See comment 0.
[List of other uplifts needed for the feature/fix]:None.
[Is the change risky?]:No.
[Why is the change risky/not risky?]:It's same problem (and fix) in nature as bug 1259473). It's a very safe fix.
[String changes made/needed]:None.
Flags: needinfo?(jwwang)
Attachment #8815594 - Flags: review+
Attachment #8815594 - Flags: approval-mozilla-beta?
Comment on attachment 8815594 [details] [diff] [review]
1315631_fix_bata.patch

We can carry over the previous approval since this was just a rebase of the original patch (versus something entirely different)
Attachment #8815594 - Flags: approval-mozilla-beta?
(Reporter)

Comment 49

2 years ago
https://hg.mozilla.org/mozilla-central/rev/32f776e0c7b5
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox53: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Tracking 53- since this is fixed.
tracking-firefox53: ? → -
(Reporter)

Comment 51

2 years ago
https://hg.mozilla.org/releases/mozilla-beta/rev/0b450f6d7994
status-firefox51: affected → fixed
Flags: needinfo?(jyavenard)
Which firefox release build date would be the first including this fix?
See Also: → bug 1273769
(In reply to Jean-Yves Avenard [:jya] from comment #53)
> Which firefox release build date would be the first including this fix?

50.1 is due for release on 13-Dec.
Whiteboard: [adv-main50.1+]
This is a sec-high fixed in 50.1 but not fixed in ESR45.6, shipping at the same time. We should have an ESR45 patch for this and get this into 45.6.
Flags: needinfo?(jwwang)
Flags: needinfo?(rkothari)
(Assignee)

Comment 56

2 years ago
(In reply to Carsten Book [:Tomcat] from comment #52)
> https://hg.mozilla.org/releases/mozilla-release/rev/
> 5c481dcfb9f2b573486fbc2a774f6935c5dff65a

Can this patch apply to ESR45?
Flags: needinfo?(jwwang)
(In reply to JW Wang [:jwwang] [:jw_wang] from comment #56)
> Can this patch apply to ESR45?

Has multiple conflicts that'll need rebasing and a branch patch posted w/ approval request ASAP.
Flags: needinfo?(jwwang)
(Assignee)

Comment 58

2 years ago
Created attachment 8817324 [details] [diff] [review]
1315631_fix_esr45.patch

A patch for ESR45.
Flags: needinfo?(jwwang)
Attachment #8817324 - Flags: approval-mozilla-esr45?
Seems like we should take this now - though it it very last minute and is going to further delay the ESR build - not ideal. (Interrupted by last week's chemspill and a lot of people traveling)
It is a sec-high fixed in 50.1. ESR45.6 is the associated ESR release. We're supposed to keep the security fixes in synch.
Comment on attachment 8817324 [details] [diff] [review]
1315631_fix_esr45.patch

sec-high crash fix, we need this for ESR 45.6.0 and I would like to go to build soon as possible. Sorry, one last patch to land.
Flags: needinfo?(wkocher)
Flags: needinfo?(cbook)
Attachment #8817324 - Flags: approval-mozilla-esr45? → approval-mozilla-esr45+
https://hg.mozilla.org/releases/mozilla-esr45/rev/7dee76404ee623f61fc741ce94752f9378a52d8e
status-firefox-esr45: affected → fixed
Flags: needinfo?(wkocher)
Flags: needinfo?(cbook)
Hi Wes, we have bustage :(
Flags: needinfo?(wkocher)
JW, can you back this out or have a look to see if you know why the builds are failing?
Flags: needinfo?(jwwang)
(Assignee)

Comment 65

2 years ago
esr45? Please back it out so I can take a look.
Flags: needinfo?(jwwang)
this is a mercurial issue, how could the commit got anything to do with it?

we need a mercurial person here.
(Assignee)

Comment 68

2 years ago
Can you also show me the build errors?
/builds/slave/m-esr45-lx-0000000000000000000/build/src/dom/media/gstreamer/GStreamerReader.h:43:20: error: 'virtual nsresult mozilla::GStreamerReader::Init()' marked override, but does not override
(In reply to Jean-Yves Avenard [:jya] from comment #67)
> this is a mercurial issue, how could the commit got anything to do with it?
> 
> we need a mercurial person here.

My bad. Misread the trace
(Assignee)

Comment 71

2 years ago
Created attachment 8817558 [details] [diff] [review]
1315631_fix_esr45_v2.patch

Fix Linux build errors. Try is still running.
Attachment #8817324 - Attachment is obsolete: true
(Assignee)

Comment 72

2 years ago
Comment on attachment 8817558 [details] [diff] [review]
1315631_fix_esr45_v2.patch

Fix Linux build errors. Try looks green on Linux.
Attachment #8817558 - Flags: approval-mozilla-esr45?
Flags: needinfo?(wkocher)
Flags: needinfo?(rkothari)
status-firefox-esr45: affected → fixed
Whiteboard: [adv-main50.1+] → [adv-main50.1+][adv-esr45.6+]
Group: media-core-security → core-security-release
Comment on attachment 8817558 [details] [diff] [review]
1315631_fix_esr45_v2.patch

This landed on ESR45 branch a month ago, just updating the flag.
Attachment #8817558 - Flags: approval-mozilla-esr45? → approval-mozilla-esr45+

Updated

2 years ago
tracking-firefox-esr45: ? → 51+
tracking-firefox-esr45: 51+ → 50+
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.