Closed Bug 1158870 Opened 5 years ago Closed 5 years ago

Enable MAR verification on linux32 and 64bit by default

Categories

(Toolkit :: Application Update, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla42
Tracking Status
firefox42 --- fixed

People

(Reporter: bbondy, Assigned: spohl)

References

Details

Attachments

(5 files, 12 obsolete files)

1.03 KB, patch
spohl
: review+
Details | Diff | Splinter Review
5.17 KB, patch
spohl
: review+
Details | Diff | Splinter Review
13.66 KB, patch
spohl
: review+
Details | Diff | Splinter Review
8.08 KB, patch
spohl
: review+
Details | Diff | Splinter Review
1.27 KB, patch
nthomas
: review+
Details | Diff | Splinter Review
The work to get multi platform MAR verification was done in Bug 1158866.

This bug is simply for defining the best place possible to define MOZ_VERIFY_MAR_SIGNATURE.  We only want to define it where MARs will be signed and we don't want to effect third parties that use our code as much as possible.
Summary: Define MOZ_VERIFY_MAR_SIGNATURE to enable MAR verification on Linux → Enable MAR verification on linux32 and 64bit by default
Attached patch Option 2: Enable on linux only (obsolete) — Splinter Review
I think this is the one that we should go with. We can enable in other places when needed.

Recall from nthomas:
> To capture opt on-push and nightly builds, and debug on-push builds, the 
> top-level mozconfigs are 
>  browser/config/mozconfigs/linux32/nightly
>  browser/config/mozconfigs/linux32/debug
>  browser/config/mozconfigs/linux64/nightly
>  browser/config/mozconfigs/linux64/debug
> AFAIK no-one else is using those, but you never can tell what people copy. If 
> you want the address sanitiser builds too then add *asan*, and there are 
> other build types to check too.
>
> Those top 4 end up sourcing other mozconfigs, but all use 
> build/unix/mozconfig.linux at some point. Except, so do b2g, xulrunner, and
> l10n (possibly harmlessly!). To cover all platforms you may wish to use
> build/mozconfig.common.
Attachment #8598391 - Flags: review?(robert.strong.bugs)
Comment on attachment 8598391 [details] [diff] [review]
Option 2: Enable on linux only

For consistency please move the enabling of the other platforms to the mozconfig files. This should be reviewed by bhearsum and / or nthomas.
Attachment #8598391 - Flags: review?(robert.strong.bugs)
Attachment #8598061 - Attachment is obsolete: true
Attached patch Patch v2 (obsolete) — Splinter Review
Attachment #8598391 - Attachment is obsolete: true
Attachment #8598400 - Flags: review?(nthomas)
Attachment #8598400 - Flags: feedback?(robert.strong.bugs)
Comment on attachment 8598400 [details] [diff] [review]
Patch v2

Looks good. I think this covers all applicable cases but it would be a good thing to verify no platforms that should run the tests, etc. are overlooked.
Attachment #8598400 - Flags: feedback?(robert.strong.bugs) → feedback+
Would also e good to ask nthomas / bhearsum if they know of a way to protect distros etc. from using our certs accidentally.
Do you think it would be worthwhile to test Linux on oak for a few days?
(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #7)
> Do you think it would be worthwhile to test Linux on oak for a few days?

I've had a successful nightly oak update on Ubuntu. But will want to make sure I have another with new changes and that everything is green.
Attached patch Patch v3. (obsolete) — Splinter Review
Builds were failing on oak due to a mozconfig check failure. I think adding it to the whitelist is the way to fix that. 

This does make me concerned though that this only enables things on nightly. And so the current patch would take away win/osx once uplift happens for new versions.  Maybe there's some kind of process that handles that uplift mozconfig settings though. Please advise.

Should I be adding similar  --enable-verify-mar options in the beta and release mozconfig?  Does aurora (webdev) use nightly's mozconfig?
Attachment #8598400 - Attachment is obsolete: true
Attachment #8598400 - Flags: review?(nthomas)
Attachment #8599040 - Flags: review?(nthomas)
(In reply to Brian R. Bondy [:bbondy] from comment #9)
> Builds were failing on oak due to a mozconfig check failure. I think adding
> it to the whitelist is the way to fix that. 
>
> This does make me concerned though that this only enables things on nightly.
> And so the current patch would take away win/osx once uplift happens for new
> versions.  Maybe there's some kind of process that handles that uplift
> mozconfig settings though. Please advise.
> 
> Should I be adding similar --enable-verify-mar options in the beta and
> release mozconfig?  Does aurora (webdev) use nightly's mozconfig?

The best process is for a developer to modify all the mozconfigs at the time a new feature lands, ie also the 'beta' and release' files you've mentioned. At which point you shouldn't need the whitelist change any more. I wish I had a better solution than adding it in so many places, maybe mshal has some ideas given he has worked on these configs.

Aurora uses 'nightly' for opt builds too (it's nightly in the sense of once-a-day, rather than Nightly the branding; but then it gets use for dep opt builds too. Naming is hard, lets go shopping).

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #6)
> Would also be good to ask nthomas / bhearsum if they know of a way to protect
> distros etc. from using our certs accidentally.

Do you mean a scenario where a someone builds with MOZ_VERIFY_MAR_SIGNATURE set, then the updater will only accept mar files which just Mozilla can produce ? This is extra fun because the problem isn't apparent until you wish to update to a 2nd build.  If so, Linux distros would very likely not care due to their own packaging infrastructure, but a fork/downstream project might. AFAIK there is no cast-iron way to prevent this. We've had env variables like MOZILLA_OFFICIAL, and more recently MOZ_AUTOMATION, but I doubt you can rely on people not setting that.
Comment on attachment 8599040 [details] [diff] [review]
Patch v3.

r- because of the problems you noticed yourself. You'll need someone else to review the browser/confvars.sh part too.
Attachment #8599040 - Flags: review?(nthomas) → review-
Thanks, I guess I'll remove the whitelisting and add to beta/release as well.
(In reply to Nick Thomas [:nthomas] from comment #10)
> (In reply to Brian R. Bondy [:bbondy] from comment #9)
> > Builds were failing on oak due to a mozconfig check failure. I think adding
> > it to the whitelist is the way to fix that. 
> >
> > This does make me concerned though that this only enables things on nightly.
> > And so the current patch would take away win/osx once uplift happens for new
> > versions.  Maybe there's some kind of process that handles that uplift
> > mozconfig settings though. Please advise.
> > 
> > Should I be adding similar --enable-verify-mar options in the beta and
> > release mozconfig?  Does aurora (webdev) use nightly's mozconfig?
> 
> The best process is for a developer to modify all the mozconfigs at the time
> a new feature lands, ie also the 'beta' and release' files you've mentioned.
> At which point you shouldn't need the whitelist change any more. I wish I
> had a better solution than adding it in so many places, maybe mshal has some
> ideas given he has worked on these configs.

The most recent changes I've been making to mozconfigs have generally been done in the common mozconfigs, so beta+release pull those in automatically. For example, you may just want to add --enable-verify-mar to build/unix/mozconfig.linux to cover the linux platforms. This would get beta+release as well, though I don't know if it would enable it for any platforms you'd like to specifically exclude. Other platforms have similar common files.
Attached patch Patch v4. (obsolete) — Splinter Review
For the confvars.sh change it's just a removal of the same options being set in the mozconfigs now. rstrong already feedback+'ed that so I'll imply an r+ for that portion of the patch.
Attachment #8599040 - Attachment is obsolete: true
Attachment #8600213 - Flags: review?(nthomas)
Sorry to have neglected the review here. I realized that we may have to consider the l10n builds as well, although it's a bit weird because we're not compiling or running tests there. Does --enable-verify-mar have any effect in the l10n case ? Maybe not because we're already adding the channel id to the mar (by way of using the mar utility from the en-US build), and signing is also the same.
> Does --enable-verify-mar have any effect in the l10n case ?

I'm not sure if the l10n case is a repack or a full rebuild of the binaries. It only has effects on the updater binary itself. If specified it'll verify the MAR file's signature before extracting it.  Robert may know better than me about the l10n case.
Comment on attachment 8600213 [details] [diff] [review]
Patch v4.

I think l10n should be OK, we download mar/mbsdiff from the matching en-US build where the mozconfig is set. r+ on the mozconfig part.
Attachment #8600213 - Flags: review?(nthomas) → review+
Comment on attachment 8600213 [details] [diff] [review]
Patch v4.

Thx, going to remove the marking so no one accidentally lands for now until enabled on oak and tested more there.
Attachment #8600213 - Flags: review+
Brian, are you going to land this or is there more to be done?
Flags: needinfo?(netzen)
I think there was some a mozconfig check or something like that failing for some reason.  Probably pretty small but I'm going to unassign myself for now.  All of the platform work is done and passing tests it's just a matter of mozconfigs.
Assignee: netzen → nobody
Flags: needinfo?(netzen)
Since mar signing isn't available on gonk I'm tempted to change
http://mxr.mozilla.org/mozilla-central/source/toolkit/xre/nsUpdateDriver.cpp#405 and other instances in this file

http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/update/updater/updater.cpp#118 and other instances in this file

#if defined(MOZ_VERIFY_MAR_SIGNATURE) && !defined(XP_WIN) && !defined(XP_MACOSX)
to
#if defined(MOZ_VERIFY_MAR_SIGNATURE) && !defined(XP_WIN) && !defined(XP_MACOSX) && !defined(MOZ_WIDGET_GONK)

When they have mar signing and if they can use that code path then it can be removed.

Also, not sure if the moconfig changes I made are acceptable but I got the mozconfig check failure to pass on oak. I also enabled marVersionDowngrade.js and marWrongChannel.js app update xpcshell tests
(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #22)
> Since mar signing isn't available on gonk I'm tempted to change
> http://mxr.mozilla.org/mozilla-central/source/toolkit/xre/nsUpdateDriver.
> cpp#405 and other instances in this file
> 
> http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/update/updater/
> updater.cpp#118 and other instances in this file
> 
> #if defined(MOZ_VERIFY_MAR_SIGNATURE) && !defined(XP_WIN) &&
> !defined(XP_MACOSX)
> to
> #if defined(MOZ_VERIFY_MAR_SIGNATURE) && !defined(XP_WIN) &&
> !defined(XP_MACOSX) && !defined(MOZ_WIDGET_GONK)
> 
> When they have mar signing and if they can use that code path then it can be
> removed.

Would you like me to go ahead and make this change as part of this patch?

> Also, not sure if the moconfig changes I made are acceptable but I got the
> mozconfig check failure to pass on oak. I also enabled
> marVersionDowngrade.js and marWrongChannel.js app update xpcshell tests

Have these mozconfig changes landed everywhere, or will I need to land those as well?
Flags: needinfo?(robert.strong.bugs)
(In reply to Stephen A Pohl [:spohl] from comment #23)
> (In reply to Robert Strong [:rstrong] (use needinfo to contact me) from
> comment #22)
> > Since mar signing isn't available on gonk I'm tempted to change
> > http://mxr.mozilla.org/mozilla-central/source/toolkit/xre/nsUpdateDriver.
> > cpp#405 and other instances in this file
> > 
> > http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/update/updater/
> > updater.cpp#118 and other instances in this file
> > 
> > #if defined(MOZ_VERIFY_MAR_SIGNATURE) && !defined(XP_WIN) &&
> > !defined(XP_MACOSX)
> > to
> > #if defined(MOZ_VERIFY_MAR_SIGNATURE) && !defined(XP_WIN) &&
> > !defined(XP_MACOSX) && !defined(MOZ_WIDGET_GONK)
> > 
> > When they have mar signing and if they can use that code path then it can be
> > removed.
> 
> Would you like me to go ahead and make this change as part of this patch?
Yes. If possible get bbondy's input.

> 
> > Also, not sure if the moconfig changes I made are acceptable but I got the
> > mozconfig check failure to pass on oak. I also enabled
> > marVersionDowngrade.js and marWrongChannel.js app update xpcshell tests
> 
> Have these mozconfig changes landed everywhere, or will I need to land those
> as well?
They have only landed on oak. You'll need to get review on them from releng (perhaps nthomas).
Flags: needinfo?(robert.strong.bugs)
Assignee: nobody → spohl.mozilla.bugs
Attached patch Patch v4. (obsolete) — Splinter Review
Updated for current trunk. Carrying over r+ by nthomas.
Attachment #8600213 - Attachment is obsolete: true
Attachment #8630638 - Flags: review+
Attached patch Fix mozconfig check failures (obsolete) — Splinter Review
These are rstrong's mozconfig changes to fix the mozconfig check failures (discussed at the bottom of comment 22 and comment 24).
Attachment #8630644 - Flags: review?(nthomas)
Note that I made these changes without any thought behind them beyond getting things building.

spohl, it would be a good thing to verify that oak nightly builds update without any issues on Linux since it has been a while since bbondy verified them.
(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #27)
> spohl, it would be a good thing to verify that oak nightly builds update
> without any issues on Linux since it has been a while since bbondy verified
> them.

Yes, I was thinking I'm getting everything that we have so far into patches and reviewed. If testing shows that we need to make additional changes, I was going to capture it in a separate patch before landing.
Attached patch Enable tests (obsolete) — Splinter Review
Enable marVersionDowngrade.js and marWrongChannel.js on Windows and OSX, disable for gonk.
Attachment #8630651 - Flags: review?(robert.strong.bugs)
Comment on attachment 8630651 [details] [diff] [review]
Enable tests

nit: fix the comment so it states "Enable marVersionDowngrade.js and marWrongChannel.js on all platforms except gonk
Attachment #8630651 - Flags: review?(robert.strong.bugs) → review+
Attached patch 3. Enable testsSplinter Review
Thanks! Fixed comment, carrying over r+.
Attachment #8630651 - Attachment is obsolete: true
Attachment #8630654 - Flags: review+
BTW: not sure but Thunderbird and SeaMonkey might need some build tweaking or it might be possible to just enable it for the test updater for those tests to pass.
Attached patch Updater changes (obsolete) — Splinter Review
These are the changes discussed at the top of comment 22 and comment 24.
Attachment #8630687 - Flags: review?(netzen)
Comment on attachment 8630687 [details] [diff] [review]
Updater changes

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

Those ifdefs are mainly because the functionality is implemented in a different way on those platforms.
It'd be even better if we can fail the build when MOZ_VERIFY_MAR_SIGNATURE is defined w/ GONK so there is no failed expectations, but I'm ok with this.
Attachment #8630687 - Flags: review?(netzen) → review+
I updated from the oak-nightly after enabling MAR verification [0][1] to the latest one available at the moment[2][3]. Robert, does this cover what you wanted me to test?

Would you like me to push attachment 8630687 [details] [diff] [review] to oak and test again?

[0] https://hg.mozilla.org/projects/oak/rev/5b078c9ba78a
[1] http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2015/05/2015-05-03-04-02-15-oak/
[2] https://hg.mozilla.org/projects/oak/rev/8c72a8f8896c
[3] http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2015/07/2015-07-09-04-02-06-oak/
Flags: needinfo?(robert.strong.bugs)
Updating from May to July is a good thing but that uses the updater code from May. It would be a good thing to update from a July build to a July build as well to verify changes to the updater haven't broken the process.

I'll go ahead and push that patch to oak as well.
Flags: needinfo?(robert.strong.bugs)
(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #38)
> https://hg.mozilla.org/projects/oak/rev/9b39451ccbe2

The update from https://hg.mozilla.org/projects/oak/rev/8c72a8f8896c to the one mentioned above was successful as well. It looks like we're in good shape.
Comment on attachment 8630644 [details] [diff] [review]
Fix mozconfig check failures

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

::: browser/config/mozconfigs/win32/beta
@@ +3,5 @@
>  
>  mk_add_options MOZ_PGO=1
>  
>  ac_add_options --enable-official-branding
> +ac_add_options --enable-signmar

Could you clarify why this is being added here ? Looks like browser/confvars.sh sets MOZ_ENABLE_SIGNMAR=1 unconditionally, and our the windows on release isn't busted despite it not being set in win32/release.

I'd rather see a patch to remove --enable-signmar from everything in browser/configs/mozconfigs/, including the whitelist. See build/compare-mozconfig/compare-mozconfigs-wrapper.py for hints on how to run the comparison locally.
Attachment #8630644 - Flags: review?(nthomas) → review-
I'm assuming you've looked at again at the codecoverage, valgrind, and asan builds and confirmed there's no point turning on verification there. Are the updater tests all in xpcshell ? Looks like we do run those asan builds.
(In reply to Nick Thomas [:nthomas] from comment #40)
> Comment on attachment 8630644 [details] [diff] [review]
> Fix mozconfig check failures
> 
> Review of attachment 8630644 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/config/mozconfigs/win32/beta
> @@ +3,5 @@
> >  
> >  mk_add_options MOZ_PGO=1
> >  
> >  ac_add_options --enable-official-branding
> > +ac_add_options --enable-signmar
> 
> Could you clarify why this is being added here ?
Adding those made make check pass on Windows. 

> Looks like
> browser/confvars.sh sets MOZ_ENABLE_SIGNMAR=1 unconditionally, and our the
> windows on release isn't busted despite it not being set in win32/release.
> 
> I'd rather see a patch to remove --enable-signmar from everything in
> browser/configs/mozconfigs/, including the whitelist. See
> build/compare-mozconfig/compare-mozconfigs-wrapper.py for hints on how to
> run the comparison locally.
Looks like bug 730862 added those.

spohl, could you try to figure out if you can get make check to pass without adding those and make sure that these changes don't break bug 730862 and repackaging.
(In reply to Nick Thomas [:nthomas] from comment #41)
> I'm assuming you've looked at again at the codecoverage, valgrind, and asan
> builds and confirmed there's no point turning on verification there. Are the
> updater tests all in xpcshell ? Looks like we do run those asan builds.
The updater tests are also in mochitest-other and could be added to other suites (I've considered adding them to mochitest-browser previously). Also, asan runs xpcshell.

Please provide guidance on what builds you'd like them excluded on giving the above and how  to disable this on those builds.
spohl, if getting the mozconfig files to pass the make check continues to be a pita let's just go with changing confvars.sh and file a new build config bug to get it moved to the the mozconfig files.
Actually, let's just go with defining it in confvars.sh and file a Firefox build config bug to decide whether or not it would be better to move these into the mozconfig files. If there are 3rd parties that have issues with the build due to it being defined in confvars.sh those bugs can be duped to it and if it is decided to move them then figuring out why the compare script was failing can be figured out in that bug. Getting mar signing on Linux has been held up long enough and there are way too many other things that need to be worked on.
Flags: needinfo?(nthomas)
Attached patch Remove --enable-signmar (obsolete) — Splinter Review
(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #42)
> (In reply to Nick Thomas [:nthomas] from comment #40)
> > I'd rather see a patch to remove --enable-signmar from everything in
> > browser/configs/mozconfigs/, including the whitelist. See
> > build/compare-mozconfig/compare-mozconfigs-wrapper.py for hints on how to
> > run the comparison locally.
> Looks like bug 730862 added those.

Fair enough; later bug 903135 changed it. This patch is all I had mind, on top of oak rev 9646b3cdbb91. It passes the mozconfig checks locally, and I don't think it's going to make any difference to an actual build. If you would rather to do this in a followup that would be fine too.

Please note I'm not advocating enabling verification in confvars.sh.
(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #43)
> Please provide guidance on what builds you'd like them excluded on giving
> the above and how  to disable this on those builds.

Sorry, not trying to be difficult here. You know where the tests run and what coverage you are happy with. Lets just leave it at opt and debug.
Attached patch Patch (obsolete) — Splinter Review
This patch addresses comment 46.
Attachment #8630638 - Attachment is obsolete: true
Attachment #8630644 - Attachment is obsolete: true
Attachment #8632192 - Flags: review?(nthomas)
Comment on attachment 8632192 [details] [diff] [review]
Patch

You'll need someone in https://wiki.mozilla.org/Modules/All#Firefox for this review.
Attachment #8632192 - Flags: review?(nthomas)
Comment on attachment 8632192 [details] [diff] [review]
Patch

Robert, please feel free to forward the review request as needed. Thanks!
Attachment #8632192 - Flags: review?(robert.strong.bugs)
Comment on attachment 8632005 [details] [diff] [review]
Remove --enable-signmar

Rotted.
Attachment #8632005 - Attachment is obsolete: true
Comment on attachment 8630638 [details] [diff] [review]
Patch v4.

After discussion with nthomas, it seems best to go back to this approach. Oak is green with these changes (see https://treeherder.mozilla.org/#/jobs?repo=try&revision=7ebbcf47b16f).
Attachment #8630638 - Attachment is obsolete: false
Attachment #8632192 - Attachment is obsolete: true
Attachment #8632192 - Flags: review?(robert.strong.bugs)
(In reply to Stephen A Pohl [:spohl] from comment #53)
> Oak is green with these changes

s/Oak/Try/
This includes two minor whitespace fixes that landed on oak. Carrying over r+.
Attachment #8630687 - Attachment is obsolete: true
Attachment #8634091 - Flags: review+
Attached patch 1. Patch v4.Splinter Review
Updated for current trunk.
Attachment #8630638 - Attachment is obsolete: true
Attachment #8634104 - Flags: review+
Nick, I've updated your patch[1] to apply to current trunk on top of the other three patches in this bug. Could you advise who could/should review this? Thanks!

[1] https://hg.mozilla.org/try/raw-rev/f6a6d3390ce4
Flags: needinfo?(nthomas)
Comment on attachment 8634107 [details] [diff] [review]
Remove --enable-signmar from mozconfigs

lgtm
Flags: needinfo?(nthomas)
Attachment #8634107 - Flags: review+
Updated commit message. Carrying over r+.
Attachment #8634107 - Attachment is obsolete: true
Attachment #8634497 - Flags: review+
Attachment #8634104 - Attachment description: Patch v4. → 1. Patch v4.
Attachment #8634091 - Attachment description: Updater changes → 2. Updater changes
Attachment #8630654 - Attachment description: Enable tests → 3. Enable tests
This followup was necessary to make 'make check' happy on Windows.

Try run without this patch (showing the failures): https://treeherder.mozilla.org/#/jobs?repo=try&revision=da91cc9a42ac
Try run with this patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3cf947510b89

Nick, I had a look at all the failures in this second try run and couldn't identify any that were related to the changes here. Is that right, or did I miss anything?
Attachment #8634503 - Flags: review?(nthomas)
Attachment #8634503 - Flags: review?(nthomas) → review+
The comparison is using a diff, so ordering matters. :-S   None of the other failures seems related to this work.
Robert, does this look ready to go to you? Shall we land and test on oak again before landing?
Flags: needinfo?(robert.strong.bugs)
Pushed to oak
Flags: needinfo?(robert.strong.bugs)
Looks like we're running into signing issues for our nightlies:
https://treeherder.mozilla.org/#/jobs?repo=oak&revision=29457cebfc44
So far, I've been unable to narrow down the cause of the failures. At least on linux we seem to be failing the signing step of make-sdk, but I'm not sure why signing is failing. Nick, do you know if there are additional log files on the signing server that could help us here?
Flags: needinfo?(nthomas)
Sorry, you were bitten by bug 1139929. I suggest you rebuild the (desktop) nightly jobs on 08f8bc16527c using treeherder.
Flags: needinfo?(nthomas)
I've updated oak to latest m-c so it will kick off a new set of nightly builds tonight.
Thanks, Nick and Robert! The updates from 7/15 to 7/17 and 7/17 to 7/20 oak nightly were successful. Shall we merge m-c to oak one last time and run through one more update, or are we ready to land at this point?
Flags: needinfo?(robert.strong.bugs)
Should be safe to land
Flags: needinfo?(robert.strong.bugs)
You need to log in before you can comment on or make changes to this bug.