Closed Bug 1368920 Opened 7 years ago Closed 7 years ago

Permaorange in browser_880382_drag_wide_widgets_in_panel.js when Gecko 55 merges to beta on 2017-06-12

Categories

(Firefox :: Toolbars and Customization, defect)

Product:
Firefox
Component:
Toolbars and Customization
Version:
55 Branch
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 56
Tracking Flags:
Tracking Status
firefox-esr45 --- unaffected
firefox-esr52 --- unaffected
firefox53 --- unaffected
firefox54 --- unaffected
firefox55 + fixed
firefox56 --- fixed

People

(Reporter: philor, Assigned: miketaylr)

References

Details

Attachments

(5 files)

Bug 1368920. Part 2 - Use MOZ_DEV_EDITION and NIGHTLY_BUILD to include non-Release and Beta extensions.
59 bytes, text/x-review-board-request
Gijs
: review+
Details
Bug 1368920. Part 3 - Use correct constants to identify Nightly and non-Release/Beta in CUI tests.
59 bytes, text/x-review-board-request
Gijs
: review+
Details
Bug 1368920. Part 4 - Enable Report Site Issue button for DevEdition and Nightly builds only.
59 bytes, text/x-review-board-request
Gijs
: review+
Details
Bug 1368920. Part 1 - AC_SUBST MOZ_DEV_EDITION so it can be used in moz.build.
59 bytes, text/x-review-board-request
ted
: review+
Details
1368920.patch
1.08 KB, patch
miketaylr
: review+
Details | Diff | Splinter Review 
Dunno yet whether or not it works for DevEdition, since someone broke building of trunk as DevEdition, but for beta, the result of bug 1341438 is going to be permaorange like https://treeherder.mozilla.org/logviewer.html#?job_id=103255770&repo=try

[Tracking Requested - why for this release]: merge bustage, closed tree, delayed b1
Mike's "(traveling May 30 - June 5 for a conf)" makes me nervous.
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to Phil Ringnalda (:philor) from comment #1)
> Mike's "(traveling May 30 - June 5 for a conf)" makes me nervous.

We explicitly trypushed to avoid this scenario. What was wrong with the trypush in bug 1341438 (for which we went and got advice in #developers ) and how do we reproduce this?
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(philringnalda)
Tracking 55+ to avoid this permaorange.
tracking-firefox55: ? → +
That try push is the patch on current beta, Gecko 54, so I'd guess based on it that if you were to land on the current beta you would be fine (at least for DevEdition, dunno either way about beta itself). But this is a super extra extended cycle, and 55 has a lot of changes from 54, apparently including some toolbar surprises, and 55 as beta and 55 as DevEdition don't work.

Once I chased down the DevEdition build bustage, for it there's not only the same set of browser_880382_drag_wide_widgets_in_panel.js failures but also a pair of browser_876944_customize_mode_create_destroy.js "number of placeholder" failures - https://treeherder.mozilla.org/logviewer.html#?job_id=103311262&repo=try.

The easy way repro, equivalent of that try push, is to (on m-c tip, you need to be above 925230851743 to build) change /config/milestone.txt from 55.0a1 to 55.0 and push to try with "try: -b o -p linux64-devedition-nightly -u mochitest-bc -t none", but since I've already done the mozharness patch to trick try into doing Mac and Windows as DevEdition (even though they won't say that's what they are) it's actually easier to just push to try on top of https://hg.mozilla.org/try/rev/c34901961b3991910b2f576d3385ee0851ed3915 with "try: -b o -p linux-devedition-nightly,linux64-devedition-nightly,macosx64,win32,win64 -u all[-10.6,-Windows XP] -t all[-10.6,-Windows XP]" (trimmed down to whatever tests you actually want to run).
Flags: needinfo?(philringnalda)
(In reply to Phil Ringnalda (:philor) from comment #1)
> Mike's "(traveling May 30 - June 5 for a conf)" makes me nervous.

I've arrived in Norway a few hours ago, and speaking tomorrow morning -- I'll have time after that to work on this.
Assignee: nobody → miket
Just copying my comment here from bug 1341438; saw the follow-up bug too late.

(In reply to Mike Taylor [:miketaylr] (traveling May 30 - June 5 for a conf) from comment #20)
> Here's a linux64-devedition try run on the beta branch:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=b3a340c5c58a0ff09e1ff5dacf8a531d9f00cce2. All green
> (after pushing tweaks for test failures).

That try build has a milestone of 55.0a1. Looks like it was branched from central, not beta. Running it shows NIGHTLY_BUILD is set, RELEASE_OR_BETA is unset, MOZ_DEV_EDITION is set.

My own DevEd builds from beta show NIGHTLY_BUILD is unset, RELEASE_OR_BETA is set, MOZ_DEV_EDITION is set.

I think the [bug 1341438] patches won't work as-is and need to check MOZ_DEV_EDITION, too.
(In reply to Jan Steffens from comment #6)
> My own DevEd builds from beta show NIGHTLY_BUILD is unset, RELEASE_OR_BETA
> is set, MOZ_DEV_EDITION is set.
> 
> I think the [bug 1341438] patches won't work as-is and need to check
> MOZ_DEV_EDITION, too.

Errr... yeah. :| I didn't realize RELEASE_OR_BETA would be set for post-Dawn Dev Edition builds.
Hi Ben, re-reading https://bugzilla.mozilla.org/show_bug.cgi?id=1353825#c44 has me a bit confused with comment #6:

> My own DevEd builds from beta show NIGHTLY_BUILD is unset, RELEASE_OR_BETA is set, MOZ_DEV_EDITION is set.

Is this expected?

Your comment in Bug 1353825 says: "* Ensure that RELEASE_OR_BETA is not set for DevEdition."

Before I write patches to fix this bug, which state of the universe is correct?
Flags: needinfo?(bhearsum)
I guess reading more into init.configure, if we're using a GRE_MILESTONE of 55, the above is expected. But if we're using 55a, then it's not expected. Phil, do you know for sure if post-aurora DevEdition builds won't have "a" in the milestone?
Flags: needinfo?(philringnalda)
(In reply to Mike Taylor [:miketaylr] (traveling May 30 - June 5 for a conf) from comment #8)
> Hi Ben, re-reading https://bugzilla.mozilla.org/show_bug.cgi?id=1353825#c44
> has me a bit confused with comment #6:
> 
> > My own DevEd builds from beta show NIGHTLY_BUILD is unset, RELEASE_OR_BETA is set, MOZ_DEV_EDITION is set.
> 
> Is this expected?
> 
> Your comment in Bug 1353825 says: "* Ensure that RELEASE_OR_BETA is not set
> for DevEdition."
> 
> Before I write patches to fix this bug, which state of the universe is
> correct?

Sorry for the delay. I believe the only reason we were looking at unsetting RELEASE_OR_BETA for DevEdition was to ensure that diagnostic assertions were enabled. I believe Sylvestre has found a different way to do that. If that's the case, we should expect that RELEASE_OR_BETA is set for DevEdition.

Sylvestre, is that right?
Flags: needinfo?(bhearsum) → needinfo?(sledru)
Right, the current patch in bug 1368079 is to make the condition for diagnostic assert NIGHTLY_BUILD || MOZ_DEV_EDITION.

So for most things there are two sets, NIGHTLY_BUILD and RELEASE_OR_BETA (the set of devedition, beta, release, esr), and for things like webcompat that are making devedition like nightly and for people who need the condition "my diagnostic assert is going to fire" there are the two sets NIGHTLY_BUILD || MOZ_DEV_EDITION and !(NIGHTLY_BUILD || MOZ_DEV_EDITION) (or the two sets RELEASE_OR_BETA && !MOZ_DEV_EDITION and else).
Flags: needinfo?(philringnalda)
(In reply to Mike Taylor [:miketaylr] (55 Engineering Regression Owner) from comment #12)
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=4ee904af8484c030497e51286ad4f90586a7eb0a

Lots of orange there, but it still seems totally unrelated, AFAICT. O-o

Locally, I've confirmed the patches pass all CUI tests when built as devedition and nightly (tweaking milestone.txt and building --with-branding=browser/branding/aurora as relevant).

I guess the tests were failing before mostly because MOZ_DEV_EDITION doesn't exist when we're preprocessing browser/extensions/moz.build -- it just knows about RELEASE_OR_BETA and NIGHTLY_BUILD. So these patches bundle the extension code for all releases, and enable the pref only for MOZ_DEV_EDITION and NIGHTLY_BUILD. 

(If it's possible to restrict the extension files to RELEASE_OR_BETA *and* MOZ_DEV_EDITION, I'd love to know how to do that...).
Comment on attachment 8875016 [details]
Bug 1368920. Part 2 - Use MOZ_DEV_EDITION and NIGHTLY_BUILD to include non-Release and Beta extensions.

https://reviewboard.mozilla.org/r/146366/#review150692

::: browser/extensions/moz.build:14
(Diff revision 1)
>      'e10srollout',
>      'pdfjs',
>      'pocket',
>      'screenshots',
>      'webcompat',
> +    'webcompat-reporter',

Shipping useless stuff on release/beta is not great. Can we avoid this? Could we just ship this if MOZ_DEV_EDITION || not CONFIG['RELEASE_OR_BETA'] is true?
Attachment #8875016 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8875018 [details]
Bug 1368920. Part 4 - Enable Report Site Issue button for DevEdition and Nightly builds only.

https://reviewboard.mozilla.org/r/146370/#review150694
Attachment #8875018 - Flags: review?(gijskruitbosch+bugs) → review+
Comment on attachment 8875017 [details]
Bug 1368920. Part 3 - Use correct constants to identify Nightly and non-Release/Beta in CUI tests.

https://reviewboard.mozilla.org/r/146368/#review150696
Attachment #8875017 - Flags: review?(gijskruitbosch+bugs) → review+
Comment on attachment 8875016 [details]
Bug 1368920. Part 2 - Use MOZ_DEV_EDITION and NIGHTLY_BUILD to include non-Release and Beta extensions.

https://reviewboard.mozilla.org/r/146366/#review150692

> Shipping useless stuff on release/beta is not great. Can we avoid this? Could we just ship this if MOZ_DEV_EDITION || not CONFIG['RELEASE_OR_BETA'] is true?

... or MOZ_DEV_EDITION || NIGHTLY_BUILD, I'm not fussed.
(In reply to :Gijs from comment #20)
> Comment on attachment 8875016 [details]
> Bug 1368920. Part 1 - Include webcompat-reporter extension assets for all
> builds.
> 
> https://reviewboard.mozilla.org/r/146366/#review150692
> 
> > Shipping useless stuff on release/beta is not great. Can we avoid this? Could we just ship this if MOZ_DEV_EDITION || not CONFIG['RELEASE_OR_BETA'] is true?
> 
> ... or MOZ_DEV_EDITION || NIGHTLY_BUILD, I'm not fussed.

Hm, not sure that's possible.

If I log the value of CONFIG['MOZ_DEV_EDITION'] in browser/extensions/moz.build:

warning('xxxxxxx')
warning('MOZ_DEV_EDITION: {}'.format(CONFIG['MOZ_DEV_EDITION']))
warning('NIGHTLY_BUILD: {}'.format(CONFIG['NIGHTLY_BUILD']))
warning('RELEASE_OR_BETA: {}'.format(CONFIG['RELEASE_OR_BETA']))

Depending on the value of milestone.txt (if we have a1 or not), NIGHTLY_BUILD and RELEASE_OR_BETA are correctly set, or None. But MOZ_DEV_EDITION is *always* None.

 0:14.19 WARNING: xxxxxxx
 0:14.19 WARNING: MOZ_DEV_EDITION: None
 0:14.19 WARNING: NIGHTLY_BUILD: None
 0:14.19 WARNING: RELEASE_OR_BETA: 1

Tomcat (or anyone else who might know....), is it possible get read MOZ_DEV_EDITION in a moz.build, or is it too early?
Flags: needinfo?(sledru) → needinfo?(cbook)
Ryan: do you know ?
Flags: needinfo?(cbook) → needinfo?(ryanvm)
(In reply to Mike Taylor [:miketaylr] (55 Regression Engineering Owner) from comment #21)
> (In reply to :Gijs from comment #20)
> > Comment on attachment 8875016 [details]
> > Bug 1368920. Part 1 - Include webcompat-reporter extension assets for all
> > builds.
> > 
> > https://reviewboard.mozilla.org/r/146366/#review150692
> > 
> > > Shipping useless stuff on release/beta is not great. Can we avoid this? Could we just ship this if MOZ_DEV_EDITION || not CONFIG['RELEASE_OR_BETA'] is true?
> > 
> > ... or MOZ_DEV_EDITION || NIGHTLY_BUILD, I'm not fussed.
> 
> Hm, not sure that's possible.
> 
> If I log the value of CONFIG['MOZ_DEV_EDITION'] in
> browser/extensions/moz.build:
> 
> warning('xxxxxxx')
> warning('MOZ_DEV_EDITION: {}'.format(CONFIG['MOZ_DEV_EDITION']))
> warning('NIGHTLY_BUILD: {}'.format(CONFIG['NIGHTLY_BUILD']))
> warning('RELEASE_OR_BETA: {}'.format(CONFIG['RELEASE_OR_BETA']))
> 
> Depending on the value of milestone.txt (if we have a1 or not),
> NIGHTLY_BUILD and RELEASE_OR_BETA are correctly set, or None. But
> MOZ_DEV_EDITION is *always* None.
> 
>  0:14.19 WARNING: xxxxxxx
>  0:14.19 WARNING: MOZ_DEV_EDITION: None
>  0:14.19 WARNING: NIGHTLY_BUILD: None
>  0:14.19 WARNING: RELEASE_OR_BETA: 1
> 
> Tomcat (or anyone else who might know....), is it possible get read
> MOZ_DEV_EDITION in a moz.build, or is it too early?

I'm not sure how to do this, sorry :(. Maybe a question for a build system person?
ted, gps, either of you know if it's possible to read MOZ_DEV_EDITION from moz.build (see Comment #21)?
Flags: needinfo?(ted)
Flags: needinfo?(gps)
We do AC_DEFINE(MOZ_DEV_EDITION) in old-configure.in. Would we just need to add an appropriate AC_SUBST while we're at it?
Flags: needinfo?(ryanvm)
(In reply to Ryan VanderMeulen [:RyanVM] from comment #25)
> We do AC_DEFINE(MOZ_DEV_EDITION) in old-configure.in. Would we just need to
> add an appropriate AC_SUBST while we're at it?

Hm, seems like that fixes it. Lemme push a try build, no idea what else might break as a result.
Comment on attachment 8875016 [details]
Bug 1368920. Part 2 - Use MOZ_DEV_EDITION and NIGHTLY_BUILD to include non-Release and Beta extensions.

https://reviewboard.mozilla.org/r/146366/#review150854

::: browser/locales/Makefile.in:106
(Diff revision 2)
>  ifndef RELEASE_OR_BETA
>  	@$(MAKE) -C ../extensions/formautofill/locale AB_CD=$* XPI_NAME=locale-$*
>  endif
>  	@$(MAKE) -C ../extensions/pocket/locale AB_CD=$* XPI_NAME=locale-$*
>  ifndef RELEASE_OR_BETA
>  	@$(MAKE) -C ../extensions/presentation/locale AB_CD=$* XPI_NAME=locale-$*

Feels like if we're updating the entire if in the other bit (so ensuring we continue to ship formautofill, presentation, etc.), we should do the same here (ie also update those same items to not be behind ifndef RELEASE_OR_BETA).
Attachment #8875016 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8875374 [details]
Bug 1368920. Part 1 - AC_SUBST MOZ_DEV_EDITION so it can be used in moz.build.

https://reviewboard.mozilla.org/r/146804/#review150866

I was going to suggest we move this to moz.configure while we're here, but I guess we don't currently have anything hooked up to branding-specific values from moz.configure.
Attachment #8875374 - Flags: review?(ted) → review+
Comment on attachment 8875016 [details]
Bug 1368920. Part 2 - Use MOZ_DEV_EDITION and NIGHTLY_BUILD to include non-Release and Beta extensions.

https://reviewboard.mozilla.org/r/146366/#review151224

rs=me in that this looks sane to me.

I have to admit my makefile-fu is super weak. The change there looks sane to me, and I guess the proof will be in the proverbial pudding (ie this builds 'properly' when landed / on try).
Attachment #8875016 - Flags: review?(gijskruitbosch+bugs) → review+
Thanks Gijs.

Looking at the latest try run, at least formautofill tests are failing (among other unrelated thing)... 

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b462adc477a0a0650674293305812c65390b1f9f&selectedJob=105389723

I just pinged MattN to ask if that addon should be included for DevEdition, but he said it should be Nightly only. So, rather than make assumptions about other addon code.... I'm gonna update my patches to *only* include webcompat-reporter for DevEdition and Nightly. 

(I also just pinged Osmose, and he has a pending patch to enable shield-recipe-client by default, so yeah, I'm gonna stop touching other people's stuff).
(In reply to Mike Taylor [:miketaylr] (55 Regression Engineering Owner) from comment #38)
> Thanks Gijs.
> 
> Looking at the latest try run, at least formautofill tests are failing
> (among other unrelated thing)... 
> 
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=b462adc477a0a0650674293305812c65390b1f9f&selectedJob=1
> 05389723
> 
> I just pinged MattN to ask if that addon should be included for DevEdition,
> but he said it should be Nightly only. So, rather than make assumptions
> about other addon code.... I'm gonna update my patches to *only* include
> webcompat-reporter for DevEdition and Nightly. 
> 
> (I also just pinged Osmose, and he has a pending patch to enable
> shield-recipe-client by default, so yeah, I'm gonna stop touching other
> people's stuff).

I'm a little confused though, because the code you're changing was already ifndef RELEASE_OR_BETA, which prior to Project Dawn would have meant the stuff would be enabled on devedition/aurora? I guess it'd be worth checking what was going on for 53 aurora or something... oh well. I'm also fine with leaving everyone else to sort themselves out.
(In reply to :Gijs from comment #39)
> I'm a little confused though, because the code you're changing was already
> ifndef RELEASE_OR_BETA, which prior to Project Dawn would have meant the
> stuff would be enabled on devedition/aurora? I guess it'd be worth checking
> what was going on for 53 aurora or something... oh well. I'm also fine with
> leaving everyone else to sort themselves out.

Yeah, it's confusing to me. 

I think for formautofill, this changeset explains it: 

https://reviewboard.mozilla.org/r/146366/diff/3#index_header

They changed the relevant pref to true for nightly only (when it was true before for devedition and nightly)... I guess if the RELEASE_OR_BETA define wasn't changing for DevEdition, the code would ship to DevEdition but would just be disabled.
Flags: needinfo?(kvijayan)

Kannan, should flyweb be included in DevEdition as well?

Chun-min, should presentation extension be included in DevEdition?
Flags: needinfo?(chun.m.chang)
No for FlyWeb.
Flags: needinfo?(kvijayan)
OK, oranges seem unrelated now. Let's see if it sticks. We can file a follow up bug on the presentation extension when Chun-min responds.
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 9d83a3b69cbb -d b63a5b39d75a: rebasing 400873:9d83a3b69cbb "Bug 1368920. Part 1 - AC_SUBST MOZ_DEV_EDITION so it can be used in moz.build. r=ted"
rebasing 400874:5b56f2d25022 "Bug 1368920. Part 2 - Use MOZ_DEV_EDITION and NIGHTLY_BUILD to include non-Release and Beta extensions. r=Gijs"
merging browser/extensions/moz.build
warning: conflicts while merging browser/extensions/moz.build! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
(lol, probably a conflict w/ https://hg.mozilla.org/integration/autoland/rev/8344aaa080f4 which just landed on autoland... I'll fix this in a few hours)
Pushed by mitaylor@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d5c7a8389ed9
Part 1 - AC_SUBST MOZ_DEV_EDITION so it can be used in moz.build. r=ted
https://hg.mozilla.org/integration/autoland/rev/3ef6dbeeef35
Part 2 - Use MOZ_DEV_EDITION and NIGHTLY_BUILD to include non-Release and Beta extensions. r=Gijs
https://hg.mozilla.org/integration/autoland/rev/a2380d694ba0
Part 3 - Use correct constants to identify Nightly and non-Release/Beta in CUI tests. r=Gijs
https://hg.mozilla.org/integration/autoland/rev/0a3b78002ba3
Part 4 - Enable Report Site Issue button for DevEdition and Nightly builds only. r=Gijs
Looks like the DevEdition builds are indeed fixed now, but the Beta ones are still broken :(

https://treeherder.mozilla.org/logviewer.html#?job_id=106012041&repo=try
Status: RESOLVED → REOPENED
status-firefox55: fixed → affected
Flags: needinfo?(miket)
Resolution: FIXED → ---
:((((

building beta right now to see how much of a pain this will be. hopefully not much...
Flags: needinfo?(miket)
Flags: needinfo?(chun.m.chang)
Attached patch 1368920.patch — — Splinter Review 
Ryan, can you land this on inbound (then beta....)? git cinnabar decided just now i need to rebuild metadata...... >_<

I've run the CUI tests after building beta, devedition and nightly with this patch.
Flags: needinfo?(ryanvm)
Attachment #8876823 - Flags: review+
Flags: needinfo?(ryanvm)
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6854e5bf150d
Remove webcompat-reporter-button for non-DevEdition RELEASE_OR_BETA builds in removeNonReleaseButtons. a=test-only
https://hg.mozilla.org/releases/mozilla-beta/rev/22f9d84db6dd

I still wonder if we'd be better off adjusting the test expectations depending on the product rather than removing the CUI entry, but this at least gets us green on Beta for now.
status-firefox55: affected → fixed
(In reply to Ryan VanderMeulen [:RyanVM] from comment #61)
> https://hg.mozilla.org/releases/mozilla-beta/rev/22f9d84db6dd
> 
> I still wonder if we'd be better off adjusting the test expectations
> depending on the product rather than removing the CUI entry, but this at
> least gets us green on Beta for now.

I think that's what this is doing (ie updating test expectations)? Not sure if I misunderstand what you're saying...
https://hg.mozilla.org/mozilla-central/rev/6854e5bf150d

Maybe I was misreading - I thought the patch was removing the element if it was there (so the expected number was the same either way) vs. expecting a different number of elements if it's Nightly/DevEdition vs. Beta.
Status: REOPENED → RESOLVED
Closed: 7 years ago → 7 years ago
status-firefox56: --- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: Firefox 55 → Firefox 56
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: