Closed Bug 1154960 Opened 5 years ago Closed 5 years ago

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

Categories

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

All
Android
defect
Not set

Tracking

(firefox38 fixed, firefox38.0.5 fixed, firefox39 fixed, firefox40 fixed, fennec38+)

RESOLVED FIXED
mozilla40
Tracking Status
firefox38 --- fixed
firefox38.0.5 --- fixed
firefox39 --- fixed
firefox40 --- fixed
fennec 38+ ---

People

(Reporter: mfinkle, Assigned: mfinkle)

References

Details

Attachments

(1 file)

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.
Indeed, it looks like Mark's fix in Bug 1151469 didn't work after all, so we'll need this.
tracking-fennec: --- → ?
Component: General → Build Config & IDE Support
OS: Mac OS X → Android
Hardware: x86 → All
(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.
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?
Flags: needinfo?(khuey)
Need this for 38, as per reoccurring failure symptoms reported in candidates indicated in bug 1151469.
(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.
Assignee: nobody → nalexander
Status: NEW → ASSIGNED
Simple patch just blocks the component and the manifest if building for Fennec
Assignee: nalexander → mark.finkle
Attachment #8595533 - Flags: review?(khuey)
Comment on attachment 8595533 [details] [diff] [review]
restrict-component v0.1

I'm not a build peer anymore.
Attachment #8595533 - Flags: review?(khuey) → review?(gps)
(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.
Flags: needinfo?(khuey) → needinfo?(benjamin)
(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).
(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.
(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.
Flags: needinfo?(benjamin)
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.
Attachment #8595533 - Flags: review?(gps) → review+
(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.
(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
Flags: needinfo?(mh+mozilla)
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.
Flags: needinfo?(mh+mozilla)
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
Attachment #8595533 - Flags: approval-mozilla-beta?
Attachment #8595533 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/bef3ac32b648
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 40
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)
Attachment #8595533 - Flags: approval-mozilla-release+
Attachment #8595533 - Flags: approval-mozilla-beta?
Attachment #8595533 - Flags: approval-mozilla-aurora?
Attachment #8595533 - Flags: approval-mozilla-aurora+
tracking-fennec: ? → 38+
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
Blocks: 1167208
Product: Firefox for Android → Firefox Build System
Target Milestone: Firefox 40 → mozilla40
You need to log in before you can comment on or make changes to this bug.