1154960 - For completeness, Fennec should explicitly block the DOM SiteSpecificUserAgent.js file from packaging

Status: RESOLVED FIXED

(Firefox Build System :: Android Studio and Gradle Integration, defect)

Description

[Mark Finkle (:mfinkle) (use needinfo?)]

Bug 1151469 added a fix that might be not a real fix. We are likely still running the risk of a packaging race.

We should just explicitly block the DOM SiteSpecificUserAgent.js file from being packaged in dom/base/moz.build EXTRA_COMPONENTS section.

Comment 1

[Richard Newman [:rnewman]]

Indeed, it looks like Mark's fix in Bug 1151469 didn't work after all, so we'll need this.

Comment 2

[Nick Alexander :nalexander [he/him]]

(In reply to Mark Finkle (:mfinkle) from comment #0)
> Bug 1151469 added a fix that might be not a real fix. We are likely still
> running the risk of a packaging race.
> 
> We should just explicitly block the DOM SiteSpecificUserAgent.js file from
> being packaged in dom/base/moz.build EXTRA_COMPONENTS section.

Coming late to this party, but it is my understanding that there is a way to override component registrations.  I am not confident that there is a way to *order* component registrations within m-c, however; we could read.

Comment 3

[Nick Alexander :nalexander [he/him]]

khuey: in Bug 1151469 we witnessed and sorta-kinda fixed a build system race where-in SiteSpecificUserAgent.{manifest,js} races to be installed from dom/base or mobile/android.

It's not clear how to override the current component registration.  Is the right solution here to remove the default registration from dom/base and move it into the individual products (browser/, b2g/, mobile/android/), so that we don't have duplicate registration?

Comment 4

[Aaron Train [:aaronmt]]

Need this for 38, as per reoccurring failure symptoms reported in candidates indicated in bug 1151469.

Comment 5

[Nick Alexander :nalexander [he/him]]

(In reply to Aaron Train [:aaronmt] from comment #4)
> Need this for 38, as per reoccurring failure symptoms reported in candidates
> indicated in bug 1151469.

A proper solution for 38 is risky, so I guess we add a conditional flag (!Fennec) to dom/base for 38.  I'll make a patch.

Comment 6

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: restrict-component v0.1

Simple patch just blocks the component and the manifest if building for Fennec

Comment 7

[Mark Finkle (:mfinkle) (use needinfo?)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4e080ba0d855

Comment 8

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

Comment on attachment 8595533 [details] [diff] [review]
restrict-component v0.1

I'm not a build peer anymore.

Comment 9

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

(In reply to Nick Alexander :nalexander from comment #3)
> khuey: in Bug 1151469 we witnessed and sorta-kinda fixed a build system race
> where-in SiteSpecificUserAgent.{manifest,js} races to be installed from
> dom/base or mobile/android.
> 
> It's not clear how to override the current component registration.  Is the
> right solution here to remove the default registration from dom/base and
> move it into the individual products (browser/, b2g/, mobile/android/), so
> that we don't have duplicate registration?

I think this is a question for bsmedberg.

Comment 10

[Mike Hommey [:glandium]]

(In reply to Nick Alexander :nalexander from comment #3)
> It's not clear how to override the current component registration.  Is the
> right solution here to remove the default registration from dom/base and
> move it into the individual products (browser/, b2g/, mobile/android/), so
> that we don't have duplicate registration?

The "right" way, that doesn't involve disabling the one in dom/base/, is to use a different file name, and ensure its manifest is registered after the dom/base/ one (which can be controlled by file order in the package manifest).

Comment 11

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Mike Hommey [:glandium] from comment #10)
> (In reply to Nick Alexander :nalexander from comment #3)
> > It's not clear how to override the current component registration.  Is the
> > right solution here to remove the default registration from dom/base and
> > move it into the individual products (browser/, b2g/, mobile/android/), so
> > that we don't have duplicate registration?
> 
> The "right" way, that doesn't involve disabling the one in dom/base/, is to
> use a different file name, and ensure its manifest is registered after the
> dom/base/ one (which can be controlled by file order in the package
> manifest).

I don't want to rename the file in mobile. Let's get a dom person to rename the dom version. Personally, I think renaming a file is the wrong solution.

Comment 12

[Nick Alexander :nalexander [he/him]]

(In reply to Mark Finkle (:mfinkle) from comment #11)
> (In reply to Mike Hommey [:glandium] from comment #10)
> > (In reply to Nick Alexander :nalexander from comment #3)
> > > It's not clear how to override the current component registration.  Is the
> > > right solution here to remove the default registration from dom/base and
> > > move it into the individual products (browser/, b2g/, mobile/android/), so
> > > that we don't have duplicate registration?
> > 
> > The "right" way, that doesn't involve disabling the one in dom/base/, is to
> > use a different file name, and ensure its manifest is registered after the
> > dom/base/ one (which can be controlled by file order in the package
> > manifest).
> 
> I don't want to rename the file in mobile. Let's get a dom person to rename
> the dom version. Personally, I think renaming a file is the wrong solution.

I don't really have a dog in this fight but am curious why renaming a file is the wrong solution.

Right now we *definitely* do a bad thing by including SiteSpecificUserAgent.js in our package-manifest.in without accounting for build system ordering and races.  (We do not include SiteSpecificUserAgent.manifest.)  If we just changed mobile/android's filename to not conflict, and updated our package-manifest.in to refer to the new .js file, we'd cleanly avoid that race -- with or without dom/base changes, AFAICT.  (We already register the MobileComponents.manifest, and AFAICT MobileComponents.manifest will always happen after, and therefore supercede, dom/base/*.manifest.  This is, of course, not guaranteed anywhere in the build system :/)

Since glandium has provided guidance I don't think we need smedberg's opinion.

Comment 13

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8595533 [details] [diff] [review]
restrict-component v0.1

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

Stealing gps's review, since this is simple.

::: dom/base/moz.build
@@ +387,5 @@
>      'SlowScriptDebug.js',
>      'SlowScriptDebug.manifest',
>  ]
>  
> +# Firefox for Android overrides this component

After this change, it's better to say "provides an alternate version of this component" or similar.

Comment 14

[Mike Hommey [:glandium]]

(In reply to Nick Alexander :nalexander from comment #12)
> (We already register the MobileComponents.manifest, and
> AFAICT MobileComponents.manifest will always happen after, and therefore
> supercede, dom/base/*.manifest.  This is, of course, not guaranteed anywhere
> in the build system :/)

It actually is guaranteed by the order in the package manifest.

Comment 15

[Nick Alexander :nalexander [he/him]]

(In reply to Mike Hommey [:glandium] from comment #14)
> (In reply to Nick Alexander :nalexander from comment #12)
> > (We already register the MobileComponents.manifest, and
> > AFAICT MobileComponents.manifest will always happen after, and therefore
> > supercede, dom/base/*.manifest.  This is, of course, not guaranteed anywhere
> > in the build system :/)
> 
> It actually is guaranteed by the order in the package manifest.

glandium: can you verify that this is because manifests in EXTRA_PP_COMPONENTS make it into EXTRA_MANIFESTS which eventually get written into chrome.manifest; and then package-manifest.in specifies chrome.manifest before MobileComponents.manifest [1], which lets MobileComponents.manifest supercede chrome.manifest?

[1] https://dxr.mozilla.org/mozilla-central/source/mobile/android/installer/package-manifest.in#604

Comment 16

[Mike Hommey [:glandium]]

That's not the right chrome.manifest, and it actually doesn't matter: it would only matter if SiteSpecificUserAgent.manifest was in the package manifest. But it's not anymore.

Comment 17

[Mark Finkle (:mfinkle) (use needinfo?)]

https://hg.mozilla.org/integration/fx-team/rev/bef3ac32b648

Comment 18

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 8595533 [details] [diff] [review]
restrict-component v0.1

Approval Request Comment
[Feature/regressing bug #]: Bug 1151469 is still showing racy bustage
[User impact if declined]: Broken 'Request Desktop Site'
[Describe test coverage new/current, TreeHerder]: Should bake a few days on Nightly.
[Risks and why]: Low. This file should not be used in Fennec anyway.
[String/UUID change made/needed]: None

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/bef3ac32b648

Comment 20

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8595533 [details] [diff] [review]
restrict-component v0.1

[Triage Comment]
Should be in 38 beta 8.
(m-b is for 38.0.5)

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/8b6e3f630f89

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-release/rev/da1d9ba28360

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/da1d9ba28360

Comment 24

[u421692]

Verified as fixed on Firefox 38 Beta 8(build 3)
Tested on:
http://www.whatsmyua.com/
http://people.mozilla.org/~atrain/mobile/tests/ua.html