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

Status: RESOLVED FIXED

(Core :: Audio/Video: Playback, defect, P1)

Description

[Carsten Book [:Tomcat]]

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]

Comment 1

[Carsten Book [:Tomcat]]

Attachment: bughunter stack

stack

Comment 2

[Carsten Book [:Tomcat]]

[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

Comment 3

[Carsten Book [:Tomcat]]

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

Comment 4

[Carsten Book [:Tomcat]]

Attachment: another stack this time crash in 0 xul.dll!mozilla::BufferDecoder::BeginDecoding(mozilla::TaskQueue *) [BufferDecoder.cpp:c44c01dfd264 : 35 + 0xe]

Comment 5

[Jean-Yves Avenard [:jya]]

Will look at it tomorrow.

Comment 6

[JW Wang [:jwwang] [:jw_wang]]

Attachment: 1315631_fix.patch

Comment 7

[Jean-Yves Avenard [:jya]]

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.

Comment 8

[Jean-Yves Avenard [:jya]]

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

Comment 9

[Jean-Yves Avenard [:jya]]

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.

Comment 10

[JW Wang [:jwwang] [:jw_wang]]

Attachment: 1315631_fix_v2.patch

Comment 11

[Jean-Yves Avenard [:jya]]

this need a security rate first

Comment 12

[Jean-Yves Avenard [:jya]]

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

Comment 13

[JW Wang [:jwwang] [:jw_wang]]

dom/media/ogg/OggReader.cpp is gonna be removed soon IIUC...

Comment 14

[Jean-Yves Avenard [:jya]]

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

Comment 15

[Carsten Book [:Tomcat]]

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

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Would be nice if we could get a sec rating on this.

Comment 17

[Carsten Book [:Tomcat]]

also maybe related to bug 1257921

Comment 18

[Randell Jesup [:jesup] (needinfo me)]

Could be sec-critical, if we believe someone could cause a reallocation with a different object

Comment 19

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

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

Comment 20

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

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.

Comment 21

[Jean-Yves Avenard [:jya]]

Why such delay imposed?

Do we like crashes that much?

Comment 22

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

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.

Comment 23

[Jean-Yves Avenard [:jya]]

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

[Jean-Yves Avenard [:jya]]

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

Comment 25

[Daniel Veditz [:dveditz]]

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

Comment 26

[Carsten Book [:Tomcat]]

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 27

[Ryan VanderMeulen [:RyanVM]]

Comment 20 already mentioned 50.1 being a possibility.

Comment 28

[Sylvestre Ledru [:Sylvestre]]

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.

Comment 29

[(no longer active)]

(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!  :-)

Comment 30

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Just like Sylvestre said.

Comment 31

[Jean-Yves Avenard [:jya]]

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

Comment 32

[Jean-Yves Avenard [:jya]]

:ehsan should I create a new bug for that particular task?

Comment 33

[(no longer active)]

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

Comment 34

[(no longer active)]

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!

Comment 35

[Jean-Yves Avenard [:jya]]

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.

Comment 36

[JW Wang [:jwwang] [:jw_wang]]

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

Comment 37

[(no longer active)]

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.

Comment 38

[Jean-Yves Avenard [:jya]]

(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

Comment 39

[JW Wang [:jwwang] [:jw_wang]]

Attachment: 1315631_fix_v3.patch

Address comment 34.
Please land this patch instead of 1315631_fix_v2.patch.

Comment 40

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

Can you also request uplift to ESR? Thanks.

Comment 41

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

jya, that needinfo in comment 40 might be for you.

Comment 42

[JW Wang [:jwwang] [:jw_wang]]

(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 43

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8808892 [details] [diff] [review]
1315631_fix_v2.patch

Sec-high, approved for all branches.

Comment 44

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/32f776e0c7b56e905d929efa1aa13e46849b6223

Comment 45

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/8402aa196fd69d6a978da9b8f4cc1b95c847db9e

Comment 46

[Ryan VanderMeulen [:RyanVM]]

JW is working on a rebased patch for Beta.

Comment 47

[JW Wang [:jwwang] [:jw_wang]]

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

Comment 48

[Ryan VanderMeulen [:RyanVM]]

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)

Comment 49

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/32f776e0c7b5

Comment 50

[Marcia Knous [:marcia]]

Tracking 53- since this is fixed.

Comment 51

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/0b450f6d7994

Comment 52

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-release/rev/5c481dcfb9f2b573486fbc2a774f6935c5dff65a

Comment 53

[Jean-Yves Avenard [:jya]]

Which firefox release build date would be the first including this fix?

Comment 54

[Ryan VanderMeulen [:RyanVM]]

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

Comment 55

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

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.

Comment 56

[JW Wang [:jwwang] [:jw_wang]]

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

Can this patch apply to ESR45?

Comment 57

[Ryan VanderMeulen [:RyanVM]]

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

Comment 58

[JW Wang [:jwwang] [:jw_wang]]

Attachment: 1315631_fix_esr45.patch

A patch for ESR45.

Comment 59

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

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)

Comment 60

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

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 61

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

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.

Comment 62

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

https://hg.mozilla.org/releases/mozilla-esr45/rev/7dee76404ee623f61fc741ce94752f9378a52d8e

Comment 63

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

Hi Wes, we have bustage :(

Comment 64

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

JW, can you back this out or have a look to see if you know why the builds are failing?

Comment 65

[JW Wang [:jwwang] [:jw_wang]]

esr45? Please back it out so I can take a look.

Comment 66

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/releases/mozilla-esr45/rev/d0c877363d21 for https://treeherder.mozilla.org/logviewer.html#?job_id=141171&repo=mozilla-esr45

Comment 67

[Jean-Yves Avenard [:jya]]

this is a mercurial issue, how could the commit got anything to do with it?

we need a mercurial person here.

Comment 68

[JW Wang [:jwwang] [:jw_wang]]

Can you also show me the build errors?

Comment 69

[Ryan VanderMeulen [:RyanVM]]

/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

Comment 70

[Jean-Yves Avenard [:jya]]

(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

Comment 71

[JW Wang [:jwwang] [:jw_wang]]

Attachment: 1315631_fix_esr45_v2.patch

Fix Linux build errors. Try is still running.

Comment 72

[JW Wang [:jwwang] [:jw_wang]]

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

Fix Linux build errors. Try looks green on Linux.

Comment 73

[Carsten Book [:Tomcat]]

  https://hg.mozilla.org/releases/mozilla-esr45/rev/893de7431d51a2a3579762ff70c5e6ef0ae96628

landed again

Comment 74

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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

This landed on ESR45 branch a month ago, just updating the flag.