Closed Bug 842571 Opened 12 years ago Closed 12 years ago

Update the process start directory (XCurProcD) in software-update.js library

Categories

(Mozilla QA Graveyard :: Mozmill Tests, defect, P2)

Product:
Mozilla QA Graveyard
Component:
Mozmill Tests
Type:
defect

Tracking

(firefox20 fixed, firefox21 fixed, firefox22 fixed, firefox23 fixed, firefox-esr17 fixed)

Status:
RESOLVED FIXED
Tracking Flags:
Tracking Status
firefox20 --- fixed
firefox21 --- fixed
firefox22 --- fixed
firefox23 --- fixed
firefox-esr17 --- fixed

People

(Reporter: AndreeaMatei, Assigned: mario.garbi)

References

Details

(Whiteboard: [lib] s=130225 u=enhancement c=software-update p=1)

Attachments

(4 files, 7 obsolete files)

Aurora 20.0a2 to Aurora 22.0a2 update log
37.58 KB, text/plain
Details
Beta 20.0 to 21.0 Update Log
29.51 KB, text/plain
Details
Comment patch
1.08 KB, patch
whimboo
: review-
Details | Diff | Splinter Review
Patch v1.6
1.68 KB, patch
AndreeaMatei
: review+
Details | Diff | Splinter Review
Due to this changeset: http://hg.mozilla.org/mozilla-central/rev/784b9beebe90 the return of XCurProcD is now Nightly/firefox/browser instead of Nightly/firefox. This needs a little investigation, at the moment the fallback test from updates has not failed.
status-firefox21: --- → affected
Whiteboard: [lib]
Priority: -- → P2
Whiteboard: [lib] → [lib] s=130225 u=enhancement c=software-update p=1
Assignee: nobody → mario.garbi
Status: NEW → ASSIGNED
This is the only line where we use "XCurProcD": http://hg.mozilla.org/qa/mozmill-tests/file/f1f141b9ff87/lib/software-update.js#l416 It is used to create a file in the Nightly/firefox/browser folder containing the update.status file where it writes the status and status.length values. Nightly/firefox/browser folder exists so the tests are not failing when the update.status file is created there. Please let me know if I need to provide additional information here.
Flags: needinfo?(hskupin)
Mario, before that change, XCurProcD was pointing to Nightly/firefox. In that main folder we have updates files, also updates folder. I feel that we should change to GreD that points to firefox (where I believe it used to be saved). Can you please check this with an older build, to be sure?
The important bit is where those status files are located nowadays. Depending on that we will have to adjust the folder.
Flags: needinfo?(hskupin)
2012 builds used to save the file in Nightly/firefox and we didn't had the /browser folder, 2013 builds save it in Nightly/firefox/browser folder as indicated by XCurProcD dumps. The update files are still in /firefox and not in firefox/browser.
We don't save any file here. We are just modifying it with a different status. So if update.status is still created under firefox/updates we are fine, but I wonder if there is another constant we could use to access it. Please check the application updater code which constant it uses.
Attached patch patch v1.0 (obsolete) — — Splinter Review
Linux report: http://mozmill-crowd.blargon7.com/#/update/report/66aad0ebfa7a01580628b3af4c7236c8 We've changed the parameter to GreD in order to point towards Nightly/firefox again instead of Nightly/firefox/browser as we had with XCurProcD. We only use this when "updateStatus == undefined || !updateStatus.exists()".
Attachment #719461 - Flags: review?(andreea.matei)
We are still creating update.status at "firefox/updates/0/update.status" but our old XCurProcD was pointing out towards firefox/browser and out final updateStatus.path was firefox/browser/updates/0/update.status and therefore not correct. Using GreD we are getting a correct path again. I've tested this on Linux - FF Default 21.0a1 (01.feb.2013) using dumps in the software-update.js and watching the update folder to verify that the update.status was created and deleted afterwards. I'm sorry for the poor descriptions/comments above.
As requested in last weeks ask an expert meeting please get feedback from Rob about this path if it is still in use or not. If not we can completely get rid of it.
XCurProcD is pointing towards "Nightly/firefox/browser" path lately, it used to point out towards "Nightly/firefox" as I've noticed on older Builds. This path is used in our update tests for getting the update directory: http://hg.mozilla.org/qa/mozmill-tests/file/1d4e86b6d128/lib/software-update.js#l415 Rob, can you please let us know if this path is still in use?
Flags: needinfo?(robert.bugzilla)
Bug 793767 made it so UpdRootD is now used.
Flags: needinfo?(robert.bugzilla)
(In reply to Robert Strong [:rstrong] (do not email) from comment #10) > Bug 793767 made it so UpdRootD is now used. Is that exclusively? In our tests we already use 'UpdRootD' but have a fallback to XCurProcD. Not sure why we have implemented that in the past. If we only make use of 'UpdRootD' we could safely remove the fallback.
Just took another look at Bug 793767 and it appears to be so.
Based on: http://dxr.mozilla.org/mozilla-central/xpcom/build/nsXULAppAPI.h#l124 I think that UpdRootD is only supported on Windows and that is why it was failing on Linux. Since XCurProcD is returning an incorrect path "Nightly/firefox/browser" could we use GreD instead for Linux and Mac? In case, it is not correct to use "GreD", can you please suggest what else to use for these two platforms?
Flags: needinfo?(robert.bugzilla)
You would be better off asking the dev that implemented the directory provider in Bug 793767 for other platforms.
Flags: needinfo?(robert.bugzilla)
Comment on attachment 719461 [details] [diff] [review] patch v1.0 Review of attachment 719461 [details] [diff] [review]: ----------------------------------------------------------------- Removing review request until we figure out what needs to be done here.
Attachment #719461 - Flags: review?(andreea.matei)
Attachment #719461 - Attachment is obsolete: true
Mike, can you please tell us what to use instead of UpdRootD for other platforms? Please see comment #13.
Flags: needinfo?(mh+mozilla)
That comment is outdated, UpdRootD can be used on all platforms now (but only on versions that have the change landed, so if you need backwards compatibility, you probably want to fallback to the parent directory of XREExeF).
Flags: needinfo?(mh+mozilla)
Attached patch patch v1.0 (obsolete) — — Splinter Review
I've replaced XCurProcD in the fallback with XREExeF as suggested in comment 17 and our path dumps return true. Linux report: http://mozmill-crowd.blargon7.com/#/update/report/00ba6b3792451db672d192b3682f4851 Mac report: http://mozmill-crowd.blargon7.com/#/update/report/00ba6b3792451db672d192b36831b491 Windows report: http://mozmill-crowd.blargon7.com/#/update/report/00ba6b3792451db672d192b36833157e
Attachment #721694 - Flags: review?(andreea.matei)
Comment on attachment 721694 [details] [diff] [review] patch v1.0 Review of attachment 721694 [details] [diff] [review]: ----------------------------------------------------------------- We do not need the fallback for current Nightly builds, right?
Comment on attachment 721694 [details] [diff] [review] patch v1.0 Review of attachment 721694 [details] [diff] [review]: ----------------------------------------------------------------- Right, no need for fallback.
Attachment #721694 - Flags: review?(andreea.matei) → review-
Linux Firefox 22.0a1 report: http://mozmill-crowd.blargon7.com/#/update/report/00ba6b3792451db672d192b3683adcbc The report for Linux in Comment 22 was in fact for Aurora backport (21.0a1).
Comment on attachment 722773 [details] [diff] [review] patch v1.1 Review of attachment 722773 [details] [diff] [review]: ----------------------------------------------------------------- Landed as: http://hg.mozilla.org/qa/mozmill-tests/rev/6887dcdc1f79 (default) http://hg.mozilla.org/qa/mozmill-tests/rev/ba70f79cf03f (aurora)
Attachment #722773 - Flags: review?(andreea.matei) → review+
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
status-firefox21: affected → fixed
status-firefox22: --- → fixed
Resolution: --- → FIXED
While investigating the recent failures we've noticed that UpdRootD no longer works with Mac OS and instead we rely on the fallback XCurProcD for the correct path, as we used to before this patch. Has the changeset that made it work before got backed out in the new builds of Firefox? I wonder where I could investigate this further or who should I ask for more detailed informations?
Flags: needinfo?(robert.bugzilla)
Mario, there is nothing Robert can help you with. This is totally on our side. Keep in mind that versions of Firefox before 21.0 will not have UpdRootD available. And this are versions the QA team uses to test updates from previous betas.
Status: REOPENED → ASSIGNED
Flags: needinfo?(robert.bugzilla)
With dump("\n\n Using XCurProcD: " + updateDir.path); and dump("\n\n Using UpdRootD: " + updateDir.path); placed in the If blocks of forcefallback we returned both the path and the element used to retrieve it for Beta, Aurora and Nightly on both Mac 10.6.8 and Linux Ubuntu 12.04. Linux Beta 19 - Beta 21 "Using XCurProcD: /tmp/tmpO9LALI.binary/updates/0" Linux Beta 20 - 21 "Using XCurProcD: /tmp/tmpmRqZe2.binary/updates/0" Linux Aurora 20-21 (without --channel=auroratest) "Using UpdRootD: /tmp/tmptboVXD.binary/updates/0" Linux Aurora 21 - 22 (with --channel=auroratest) "Using UpdRootD: /tmp/tmp2bIHn8.binary/updates/0" Mac 10.6.8 Nightly 22 - 23 ================================== Using UpdRootD: /var/folders/fP/fPyCuoSSEVqn8CXCucVH5E+++TM/-Tmp-/tmpHz5u99.binary/FirefoxNightly.app/Contents/MacOS/updates/0 =================================== Beta 19 - 21 ================================== Using XCurProcD: /var/folders/fP/fPyCuoSSEVqn8CXCucVH5E+++TM/-Tmp-/tmp3KsOK3.binary/Firefox.app/Contents/MacOS/updates/0 =================================== Aurora 21-22 ================================== Using UpdRootD: /var/folders/fP/fPyCuoSSEVqn8CXCucVH5E+++TM/-Tmp-/tmp00Kw7E.binary/FirefoxAurora.app/Contents/MacOS/updates/0 =================================== We should keep the fallback until our oldest version of testruns is at least 20 since UpdRootD isn't implemented before that. As it is we use UpdRootD where it applies and fallback to XCurProcD where that fails.
(In reply to mario garbi from comment #28) > Linux Beta 20 - 21 > "Using XCurProcD: /tmp/tmpmRqZe2.binary/updates/0" > > Linux Aurora 20-21 > "Using UpdRootD: /tmp/tmptboVXD.binary/updates/0" That doesn't make sense. Why is the Aurora build using the UpdRootD constant while the beta of the same version is not? If the change has been landed on Aurora it also has to be available for beta. Mike, do you have an idea? Not sure if I miss something.
Flags: needinfo?(mh+mozilla)
I'm not sure what the question is. Beta 21 has UpdRootD and has a browser/ subdirectory. Beta 20 didn't.
Flags: needinfo?(mh+mozilla)
(In reply to Mike Hommey [:glandium] from comment #30) > I'm not sure what the question is. Beta 21 has UpdRootD and has a browser/ > subdirectory. Beta 20 didn't. Exactly, but why is Aurora 20 using UpdRootD while beta 20 is using XCurProcD. Are releases handled differently? I can't imagine.
(In reply to Henrik Skupin (:whimboo) from comment #31) > Exactly, but why is Aurora 20 using UpdRootD while beta 20 is using > XCurProcD. Are releases handled differently? I can't imagine. how is it "using XCurProcD"? what is?
Mike, that was the message I used in the dump to show which element we used to retrieve the path. if (updateStatus == undefined || !updateStatus.exists()) { updateDir = Services.dirsvc.get("XCurProcD", Ci.nsIFile); updateDir.append("updates"); updateDir.append("0"); dump("\n\n Using XCurProcD: " + updateDir.path + "\n\n"); updateStatus = updateDir.clone(); updateStatus.append("update.status");
We have two blocks of code that first try to get the path using UpdRootD and if that fails we try to retrieve it using the code above. http://hg.mozilla.org/qa/mozmill-tests/file/4cffcdf65d99/lib/software-update.js#l400
Where and when is that code running, and what does "Linux Beta 20 - 21" and "Linux Aurora 20-21" mean?
I have been running testrun_update.py script with those builds of Firefox and latest mozmill-tests repository. I have added two dumps to the code as explained above in order to print the value to the console and figure which code block was used in each build case. I have those reports from today, all reports being successful, some using UpdRootD and some XCurProcD, depenting on the Firefox build was used. For Aurora to get updated to 22 I used --channel=auroratest. I have copied the values from the console log to the comment using the syntax Linux (Ubuntu 12.04) Beta 20 (Firefox Beta 20.0b4 pre-build) - 21 (Firefox Beta 21 post-build) and Mac OS 10.6.8 I am sorry if I was brief in the comments above.
Sorry for being dense, but i still don't get what versions are involved and when/where that code block is run (before or after upgrade, with the old version or the new one)
I will provide the Mozmill-crowd links for each type of test. We have added the dumps as shown in comment 33 to these locations: dump("\n\n Using UpdRootD: " + updateDir.path + "\n\n"); http://hg.mozilla.org/qa/mozmill-tests/file/4cffcdf65d99/lib/software-update.js#l409 dump("\n\n Using XCurProcD: " + updateDir.path + "\n\n"); http://hg.mozilla.org/qa/mozmill-tests/file/4cffcdf65d99/lib/software-update.js#l419 Linux Beta 19 - Beta 21 http://mozmill-crowd.blargon7.com/#/update/report/740ab23e062758ae8a240b263d4c46f3 Linux Beta 20 - 21 http://mozmill-crowd.blargon7.com/#/update/report/740ab23e062758ae8a240b263d4cc250 Linux Aurora 20-21 (without --channel=auroratest) http://mozmill-crowd.blargon7.com/#/update/report/740ab23e062758ae8a240b263d4f8b6c Linux Aurora 21 - 22 (with --channel=auroratest) http://mozmill-crowd.blargon7.com/#/update/report/740ab23e062758ae8a240b263d50093f Mac 10.6.8 Nightly 22 - 23 http://mozmill-crowd.blargon7.com/#/update/report/740ab23e062758ae8a240b263d4c8ad2 Beta 19 - 21 http://mozmill-crowd.blargon7.com/#/update/report/740ab23e062758ae8a240b263d0ab388 Aurora 21-22 http://mozmill-crowd.blargon7.com/#/update/report/740ab23e062758ae8a240b263d51a6ed This is implemented in the forceFallback() method and this is used in the tearDown parts of our testFallbackUpdate/test2.js. This happens before we update to the postbuild. If you need other informations or if there is a better way for me to explain it please let me know Mike and I will respond as fast as I can.
If that code is running before upgrades, there's no reason it should be working with UpdRootD on any version of 20.
(In reply to Mike Hommey [:glandium] from comment #39) > If that code is running before upgrades, there's no reason it should be > working with UpdRootD on any version of 20. Exactly that's why I'm wondering in comment 29 why Aurora 20.0a2 makes use of UpdRootD. Mario, please repeat that and attach the log file and NOT any report link. Those are not helpful at all.
Aurora update log with dumps
(In reply to mario garbi from comment #41) > Created attachment 733799 [details] > Aurora 20.0a2 to Aurora 22.0a2 update log That looks like I would expect it to be: Using XCurProcD: /var/folders/fP/fPyCuoSSEVqn8CXCucVH5E+++TM/-Tmp-/tmpwaFIx2.binary/FirefoxAurora.app/Contents/MacOS/updates/0 Are you sure that you initially run a Aurora 20 build which made use of UpdRootD? I would imagine that this was a 21.0a2 build.
Update log for Beta 20.0 to 21.0 builds, both here and on aurora 20 we used XCurProcD on Mac OS
Hmm I tried to reproduce that dump with multiple builds, it seems to have been a mistake on my part. I must have used a wrong aurora build indeed. Each Aurora 20 update used XCurProcD as expected and not UpdRootD. I am sorry for the confusion there. This reinforce the need for the fallback for builds that predate 21.
Good to hear. So please add code which selects the right constant dependent on the version of Firefox under test.
Attached patch patch v1.3 (obsolete) — — Splinter Review
Something was wrong with the previous patch, uploading right one now.
Attachment #733872 - Attachment is obsolete: true
Attachment #733872 - Flags: review?(andreea.matei)
Attachment #733874 - Flags: review?(andreea.matei)
Comment on attachment 733874 [details] [diff] [review] patch v1.3 Review of attachment 733874 [details] [diff] [review]: ----------------------------------------------------------------- Landed on default: http://hg.mozilla.org/qa/mozmill-tests/rev/1473e7ea15b0
Attachment #733874 - Flags: review?(andreea.matei) → review+
For version checks we should really use the Services.vc interface. Everything else can be faulty. I'm tempted to push a follow-up fix for that. See the update library how this interface works. We should be consistent.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
This is still not working and I'm not sure why the tests to qualify the lastest patches have only been ran on Ubuntu but not on Mac and Windows too. All of the ondemand update tests last night are failing on Windows! Here a quick log output: ********* path: XCurProcD ****** real path: c:\users\mozauto\appdata\local\temp\tmpxoklu_.binary\updates\0 *** AUS:SVC readStatusFile - status: applied, path: C:\Users\mozauto\AppData\Local\Mozilla\Firefox\firefox\updates\0\update.status We can't continue in landing patches without proper testing across platforms as it has been requested as a strict requirement. This massively delayed QA from signing of from releasetest testing. Andreea, please backout the landed changes across branches immediately. Thanks. Once done we should simply add a comment to those lines in the update library which mention why we still need the fallback. Any removal of the fallback will be done later when we do not have to test releases <21 anymore.
Marking as incomplete for now. There is no need to invest more time on this bug until the old paths have totally gone and we don't test those releases. Closing as incomplete for now.
Status: REOPENED → RESOLVED
Closed: 12 years ago → 12 years ago
status-firefox20: fixed → ---
status-firefox21: fixed → ---
status-firefox22: fixed → ---
status-firefox23: fixed → ---
status-firefox-esr17: fixed → ---
Resolution: --- → INCOMPLETE
Attached patch Comment patch — — Splinter Review
Added a comment in the library as suggested above that reflects the fact that we can remove the fallback once we will not use builds older than 21 in our update tests.
Attachment #736290 - Flags: review?(andreea.matei)
Attachment #736290 - Flags: review?(hskupin)
Status: RESOLVED → REOPENED
Resolution: INCOMPLETE → ---
Attachment #733874 - Attachment is obsolete: true
Comment on attachment 736290 [details] [diff] [review] Comment patch Review of attachment 736290 [details] [diff] [review]: ----------------------------------------------------------------- With the fix on bug 860703 we will be able to fix this bug for real! There is no need for just a comment. Surprisingly it came together all on one day, so this time we will be able to fix it for real.
Attachment #736290 - Flags: review?(hskupin)
Attachment #736290 - Flags: review?(andreea.matei)
Attachment #736290 - Flags: review-
Attached patch patch v1.4 (obsolete) — — Splinter Review
Reports for staginDirectory.patch (12.04.2013) Linux: 23 http://mozmill-crowd.blargon7.com/#/update/report/8ec48e7ab0431a61b624e36d3118ea60 http://mozmill-crowd.blargon7.com/#/update/report/8ec48e7ab0431a61b624e36d3118989f 22 http://mozmill-crowd.blargon7.com/#/update/report/8ec48e7ab0431a61b624e36d3119665d http://mozmill-crowd.blargon7.com/#/update/report/8ec48e7ab0431a61b624e36d3119aa42 21 http://mozmill-crowd.blargon7.com/#/update/report/8ec48e7ab0431a61b624e36d3119ef63 http://mozmill-crowd.blargon7.com/#/update/report/8ec48e7ab0431a61b624e36d311a2bd5 20 http://mozmill-crowd.blargon7.com/#/update/report/8ec48e7ab0431a61b624e36d311a5cf3 http://mozmill-crowd.blargon7.com/#/update/report/8ec48e7ab0431a61b624e36d311a83bf Windows: 23 http://mozmill-crowd.blargon7.com/#/update/report/8ec48e7ab0431a61b624e36d3119a0ec http://mozmill-crowd.blargon7.com/#/update/report/8ec48e7ab0431a61b624e36d3119e43f 22 http://mozmill-crowd.blargon7.com/#/update/report/8ec48e7ab0431a61b624e36d311ab987 http://mozmill-crowd.blargon7.com/#/update/report/8ec48e7ab0431a61b624e36d311adbd6 21 http://mozmill-crowd.blargon7.com/#/update/report/8ec48e7ab0431a61b624e36d311e597c http://mozmill-crowd.blargon7.com/#/update/report/8ec48e7ab0431a61b624e36d311ea273 20 http://mozmill-crowd.blargon7.com/#/update/report/8ec48e7ab0431a61b624e36d311b1610 http://mozmill-crowd.blargon7.com/#/update/report/8ec48e7ab0431a61b624e36d311b3338
Attachment #736787 - Flags: review?(andreea.matei)
Comment on attachment 736787 [details] [diff] [review] patch v1.4 Review of attachment 736787 [details] [diff] [review]: ----------------------------------------------------------------- One thing to change. Otherwise I would suggest to test this on your local CI instance with an ondemand testrun across different platforms. ::: lib/software-update.js @@ +410,2 @@ > var updateStatus; > + var updateDir = this.stagingDirectory; It's used once. So why do we need this variable?
Attachment #736787 - Flags: review?(andreea.matei) → review-
Comment on attachment 738919 [details] [diff] [review] Patch v1.5 Review of attachment 738919 [details] [diff] [review]: ----------------------------------------------------------------- Please add a commit message.
Attachment #738919 - Flags: review?(andreea.matei) → review-
Attached patch Patch v1.6 — — Splinter Review
I overlooked the commit message, will not happen again.
Attachment #736787 - Attachment is obsolete: true
Attachment #738919 - Attachment is obsolete: true
Attachment #740261 - Flags: review?(andreea.matei)
Comment on attachment 740261 [details] [diff] [review] Patch v1.6 Review of attachment 740261 [details] [diff] [review]: ----------------------------------------------------------------- This commit shows the changes you've made: http://hg.mozilla.org/qa/mozmill-tests/rev/02b1b2e1661b (default) Thanks.
Attachment #740261 - Flags: review?(andreea.matei) → review+
When we backport this please do it first for aurora and beta, and ensure that an ondemand update testrun with real snippet from Anthony works as expected. Then backport to release. Once pushed don't forget to manually run the mozmill-tests job in both CI.
status-firefox20: --- → affected
status-firefox21: --- → affected
status-firefox22: --- → affected
status-firefox-esr17: --- → affected
We have ran an ondemand update job on our local Jenkins for Beta and we had a Green run http://mozmill-crowd.blargon7.com/#/update/reports?branch=All&platform=All&from=2013-04-24&to=2013-04-24 We got the config file from the latest CI ondemand job for Beta.
That's fantastic news. Good to see that this solution works. Andreea, can you please push to beta as soon as you can? Once pushed we will wait for this weeks ondemand beta run for 21.0b3. If that is green, we can push to release by end of this week.
Status: REOPENED → ASSIGNED
The ondemand update testrun for Release on our local Jenkins popped a few errors we see in the CI ondemand jobs but not related to this patch (for example Bug 808550). http://mozmill-crowd.blargon7.com/#/update/reports?branch=20.0&platform=All&from=2013-04-25&to=2013-04-25
Does this still need to land to the moz-mill tests branch for esr17?
(In reply to lsblakk@mozilla.com from comment #68) > Does this still need to land to the moz-mill tests branch for esr17? Thanks Lukas. Yes, it has to land on release and esr17. Given that Anthony successfully run the update tests on beta this week we are safe now in getting this patch on those two releases. Mario, please check some updates on those branches so we can assume everything will work.
We have ondemand reports for Release branch (comment 67). I will test it today for esr17.
Last branches: http://hg.mozilla.org/qa/mozmill-tests/rev/42e56c7a5f4b (esr17) http://hg.mozilla.org/qa/mozmill-tests/rev/216a4b34d66e (release) Hopefully we're done here, thanks Mario.
Status: ASSIGNED → RESOLVED
Closed: 12 years ago → 12 years ago
status-firefox20: affected → fixed
status-firefox-esr17: affected → fixed
Resolution: --- → FIXED
Product: Mozilla QA → Mozilla QA Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: