1248590 - Make sure build promotion produces packages equivalent to release builds

Status: RESOLVED FIXED

(Release Engineering :: Release Automation: Other, defect)

Description

[Rail Aliiev [:rail]]

Apparently --enable-tests changes the way how we compile files. 

https://dxr.mozilla.org/mozilla-central/search?q=ENABLE_TESTS+path%3A.cpp&redirect=false&case=false give a lot of entries, where we use #ifdefs to compile different code.

It could be similar for other files types (Makefiles, headers, etc).

One of the ideas of build promotion is to ship the same code that we test, but this looks like a show stopper here...

Comment 1

[Joel Maher ( :jmaher ) (UTC -8)]

looking at some of this:
https://dxr.mozilla.org/mozilla-central/source/dom/media/ADTSDemuxer.cpp#196
* support for previous frame- lets dig into why this is here and why it is needed.  It was added in bug 1169212.

Dan, can you help us figure out why we have prevFrame behind ENABLE_TESTS?  Is there a reason not to put this in production builds?  If we shouldn't put this in a production build, can we solve this another way?

Comment 2

[Joel Maher ( :jmaher ) (UTC -8)]

this is also the case for https://dxr.mozilla.org/mozilla-central/source/dom/media/MP3Demuxer.cpp

Comment 3

[Joel Maher ( :jmaher ) (UTC -8)]

in bug 961049, we added enable_tests support to ActorsParent.cpp:
https://dxr.mozilla.org/mozilla-central/source/dom/quota/ActorsParent.cpp

this seems to support the disk space data while running xpcshell tests.

:janv, is this something we can put into a production build?  If not, is there a way we could test this functionality without the enable_tests?

Comment 4

[Joel Maher ( :jmaher ) (UTC -8)]

in bug 1013571, support for enable_tests in RuntimeService.cpp was added:
https://dxr.mozilla.org/mozilla-central/source/dom/workers/RuntimeService.cpp

these changes look to add test methods, I am not sure if this is something reasonable to add into a production build.

:bkelly, can you add more info here and help us determine if we can ship a release build with this test code or if we can change the code or tests to make this work?

Comment 5

[Joel Maher ( :jmaher ) (UTC -8)]

changes in:
https://dxr.mozilla.org/mozilla-central/source/toolkit/xre/nsAppRunner.cpp

come from 2 bugs, bug 844288 and bug 767231.  These are both the same area of code and adding a hook to run gtests.

:BenWa, if we are looking to ship release builds with --enable-tests defined, are these gtest changes in nsAppRunner.cpp reasonable to ship?  Are there other ways to solve this?

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Joel Maher (:jmaher) from comment #4)
> in bug 1013571, support for enable_tests in RuntimeService.cpp was added:
> https://dxr.mozilla.org/mozilla-central/source/dom/workers/RuntimeService.cpp
> 
> these changes look to add test methods, I am not sure if this is something
> reasonable to add into a production build.

This was modeled off the existing ENABLE_TESTS in ContentParent.cpp for testing PBackground.  That was introduced by :bent.

In general I think exposing test IPC messages is not a good idea for release.  An attacker could craft messages to parent using this actor.  Since its test code it does not get the scrutiny other code gets.

In theory we have enough real features using PBackground now, though, that maybe we could remove the PBackground test actor completely.  Kyle, what do you think?

Comment 7

[Jan Varga [:janv]]

We use it this way:

const bool QuotaManager::kRunningXPCShellTests =
#ifdef ENABLE_TESTS
  !!PR_GetEnv("XPCSHELL_TEST_PROFILE_DIR")
#else
  false
#endif
  ;

So quota/indexedDB code behaves differently if kRunningXPCShellTests is true which we definitely don't want for production. ENABLE_TESTS itself doesn't trigger different behavior, XPCSHELL_TEST_PROFILE_DIR must be set too. I think we can just remove the ENABLE_TESTS check in this case. It will cost us one more PR_GetEnv() call in production builds.

Comment 8

[Jan Varga [:janv]]

(In reply to Ben Kelly [:bkelly] from comment #6)
> In theory we have enough real features using PBackground now, though, that
> maybe we could remove the PBackground test actor completely.  Kyle, what do
> you think?

I think :mrbkap reviewed that PBackground code, he might have an opinion on that too.

Comment 9

[Joel Maher ( :jmaher ) (UTC -8)]

in bug 956218 we added ENABLE_TESTS support in ContentParent.cpp:
https://dxr.mozilla.org/mozilla-central/source/dom/ipc/ContentParent.cpp

:bent, can you weigh in on how we could (if possible) resolve the enable_tests such that we could ship a release build from a build we actually test.  It sounds like we don't want this code in a release build- maybe an addon or possibly we don't need these tests/code anymore?

Comment 10

[Joel Maher ( :jmaher ) (UTC -8)]

:janv, regarding XPCSHELL_TEST_PROFILE_DIR, do you want to use this current bug, or would you like me to file a new bug.  Also would you like me to create the patch, or is this something you want to do?

Comment 11

[Jan Varga [:janv]]

(In reply to Joel Maher (:jmaher) from comment #10)
> :janv, regarding XPCSHELL_TEST_PROFILE_DIR, do you want to use this current
> bug, or would you like me to file a new bug.  Also would you like me to
> create the patch, or is this something you want to do?

It should be pretty simple patch, feel free to do it if you have time, I'll review..
A new bug is not needed.

Comment 12

[Joel Maher ( :jmaher ) (UTC -8)]

I believe the last big of ENABLE_TESTS trickery is related to bug 1166207 and files:
https://dxr.mozilla.org/mozilla-central/source/ipc/glue/MessageLink.cpp
https://dxr.mozilla.org/mozilla-central/source/mozglue/build/Nuwa.cpp

:cyu, this is a lot of code executed, it doesn't look like something we want in release builds.  I do question if we are actually running tests that use this now that b2g is tier3.  Can you help us figure out if we can ship with this, we can solve this problem another way, or if we can remove this code?

Comment 13

[Justin Wood (:Callek)]

11:22 AM <Callek> jmaher: using path:.h and removing path: entirely on the ENABLE_TESTS bits is also useful, I suspect there is at least one or two things lurking in the makefiles as well (though makefiles should be generally clear)

Comment 14

[Benoit Girard (:BenWa)]

If you're asking about the conditional #include then yes it's fine. In fact I think you can remove it entirely because we forward declare what we need here anyways:
https://dxr.mozilla.org/mozilla-central/source/toolkit/xre/nsAppRunner.cpp#267

Comment 15

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

I'm not opposed to removing the PBackground testing infrastructure now that it has real in-tree consumers.

Comment 16

[Blake Kaplan (:mrbkap) (inactive)]

I agree with Kyle.

Comment 17

[Cervantes Yu [:cyu] [:cervantes]]

(In reply to Joel Maher (:jmaher) from comment #12)
> I believe the last big of ENABLE_TESTS trickery is related to bug 1166207
> and files:
> https://dxr.mozilla.org/mozilla-central/source/ipc/glue/MessageLink.cpp
> https://dxr.mozilla.org/mozilla-central/source/mozglue/build/Nuwa.cpp
> 
> :cyu, this is a lot of code executed, it doesn't look like something we want
> in release builds.  I do question if we are actually running tests that use
> this now that b2g is tier3.  Can you help us figure out if we can ship with
> this, we can solve this problem another way, or if we can remove this code?

b2g tests are still run on try and b2g-inbound. Connected device and TV are consumers of this code. I think we can make it #ifdef MOZ_NUWA_PROCESS instead.

Comment 18

[u480271]

Joel,

I cribbed off MP3Demuxer when creating ADTSDemuxer. MP3Demuxer had ENABLED_TESTS and I just copied it. The code is only an aid for debugging. I think it would be safe to remove the ENABLED_TESTS parts entirely from ADTSDemuxer and not effect functionality.

Comment 19

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

/me defers to mrbkap & khuey

Comment 20

[Joel Maher ( :jmaher ) (UTC -8)]

thanks everyone for the info- I am working on creating bugs and patches to remove ifdef statements as discussed :)

Comment 21

[Rail Aliiev [:rail]]

(In reply to Joel Maher (:jmaher) from comment #20)
> thanks everyone for the info- I am working on creating bugs and patches to
> remove ifdef statements as discussed :)

You are the best! :) TYVM!

Comment 22

[Joel Maher ( :jmaher ) (UTC -8)]

according to dxr we have 0 results:
https://dxr.mozilla.org/mozilla-central/search?q=ENABLE_TESTS+path%3A.cpp&redirect=false&case=false

looking at all other instances of ENABLE_TESTS (mostly makefile/moz.build), I see one thing to look into:
https://dxr.mozilla.org/mozilla-central/search?q=DOM_STORAGE_TESTS&redirect=false&case=false

and we have predictor_tests defined but nothing using it, :mcmanus, can that define be removed:
https://dxr.mozilla.org/mozilla-central/source/netwerk/base/moz.build#312


:honza, can you comment on if we ship a release build with DOM_STORAGE_TEST=1 is that a bad thing? For reference here is the list via dxr:
https://dxr.mozilla.org/mozilla-central/search?q=DOM_STORAGE_TESTS&redirect=false&case=false

Comment 23

[Patrick McManus [:mcmanus]]

nick is the right person to ask

Comment 24

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to Joel Maher (:jmaher) from comment #22)
> :honza, can you comment on if we ship a release build with
> DOM_STORAGE_TEST=1 is that a bad thing? For reference here is the list via
> dxr:
> https://dxr.mozilla.org/mozilla-central/
> search?q=DOM_STORAGE_TESTS&redirect=false&case=false
Jan Bambas should know more about this I guess.

Honza

Comment 25

[Honza Bambas (:mayhemer)]

(In reply to Joel Maher (:jmaher) from comment #22)
> according to dxr we have 0 results:
> https://dxr.mozilla.org/mozilla-central/search?q=ENABLE_TESTS+path%3A.
> cpp&redirect=false&case=false
> 
> looking at all other instances of ENABLE_TESTS (mostly makefile/moz.build),
> I see one thing to look into:
> https://dxr.mozilla.org/mozilla-central/
> search?q=DOM_STORAGE_TESTS&redirect=false&case=false
> 
> and we have predictor_tests defined but nothing using it, :mcmanus, can that
> define be removed:
> https://dxr.mozilla.org/mozilla-central/source/netwerk/base/moz.build#312
> 
> 
> :honza, can you comment on if we ship a release build with
> DOM_STORAGE_TEST=1

We do, I think.  Whenever tests are enabled (ENABLE_TESTS=1) at compilation time (which is IMO always) we do.

> is that a bad thing? 

No, none of this is dangerous, exploitable or exposed to content.  These test snippets are never used, just by some of the tests that run on any build we release, I presume.

> For reference here is the list via
> dxr:
> https://dxr.mozilla.org/mozilla-central/
> search?q=DOM_STORAGE_TESTS&redirect=false&case=false

Comment 26

[Joel Maher ( :jmaher ) (UTC -8)]

Thanks Honza!  I will not worry about DOM_STORAGE_TESTS ifdef and move on.

The only other code work I see is potentially removing the ifdef in necko if it isn't used- I will wait on :hurley for that, but that shouldn't block this bug.

:rail, as you filed this bug on code specific issues, can we close this?

Comment 27

[Rail Aliiev [:rail]]

There is one thing left that I wanted to look at today - packaging. I'll close as soon as we are done.

Thanks a lot!

Comment 28

[u408661]

(In reply to Joel Maher (:jmaher) from comment #22)
> and we have predictor_tests defined but nothing using it, :mcmanus, can that
> define be removed:
> https://dxr.mozilla.org/mozilla-central/source/netwerk/base/moz.build#312

Yep, that can go.

Comment 29

[Rail Aliiev [:rail]]

Joel,

We want to switch to release promotion starting 46.0b1 (go to build in ~8 days). Do you think it's worth uplifting the patches to aurora next week so they migrate to beta?

Comment 30

[Joel Maher ( :jmaher ) (UTC -8)]

yeah, I think that sounds reasonable.

Comment 31

[Joel Maher ( :jmaher ) (UTC -8)]

requested all the uplift requests.

Comment 32

[Joel Maher ( :jmaher ) (UTC -8)]

all patches are on aurora!