Closed Bug 626998 Opened 15 years ago Closed 15 years ago

Intermittent failure in browser_bug553455.js | Test timed out

Categories

(Toolkit :: Add-ons Manager, defect)

Product:
Toolkit
Component:
Add-ons Manager
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla6

People

(Reporter: mounir, Unassigned)

References

(Blocks 1 open bug)

Details

(Keywords: intermittent-failure)

Attachments

(5 files)

The listener for install dialog shoule be registered before sending mouse event [checked-in]
4.68 KB, patch
mossop
: review+
Details | Diff | Splinter Review
remove correct temporary file
8.26 KB, patch
mossop
: review-
Details | Diff | Splinter Review
Fix havoc of browser_bug592338.js [checked-in]
2.18 KB, patch
mossop
: review+
Details | Diff | Splinter Review
wait for onStopRequest [backed-out]
7.30 KB, patch
Gavin
: review+
Details | Diff | Splinter Review
Should wait for notification and http transaction
5.09 KB, patch
Details | Diff | Splinter Review
I'm assuming this failure is different from bug 579540 and bug 623242. Feel free to mark as a dupe if I'm wrong. TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/browser_bug553455.js | Test timed out TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/browser_bug553455.js | Found a tab after previous test timed out: http://example.com/browser/toolkit/mozapps/extensions/test/xpinstall/installtrigger.html?%7B%22XPI%22%3A%22unsigned.xpi%22%7D TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/browser_bug553455.js | application timed out after 330 seconds with no output http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1295443809.1295444777.15958.gz
This patch will fix the timeout on waiting for install dialog in test_blocked_install and test_cancel_restart. But there are still other failures that the timeout happens on waiting for notification popup. I am suspecting that popupshown event does not occur but I have no evidence for it yet.
Attachment #506095 - Flags: review?
Attachment #506095 - Flags: review? → review?(dtownsend)
Attachment #506095 - Flags: review?(dtownsend) → review+
Attachment #506095 - Attachment description: The listener for install dialog shoule be registered before sending mouse event → The listener for install dialog shoule be registered before sending mouse event [checked-in]
This is a fix for failing in test_cancel_restart. In the case of failing in test_cancel_start, temporary file does not exist while zipreader.open() because onStopRequest is invoked after restarting install so that the temporary file which removed in onStopRequest becomes the new one.
Attachment #512670 - Flags: review?(dtownsend)
This patch actually is not a fix for this issue but a fix for a side effect of this failure. As you can see in the log of comment 194, "popupshown" listener was still alive while running browser_bug592338.js. We should remove the listener even if timeout happens.
Attachment #512693 - Flags: review?(dtownsend)
Attachment #512693 - Flags: review?(dtownsend) → review+
Comment on attachment 512670 [details] [diff] [review] remove correct temporary file Not really following what the problem is that you're trying to fix here. > this.channel.cancel(Cr.NS_BINDING_ABORTED); > case AddonManager.STATE_AVAILABLE: > case AddonManager.STATE_DOWNLOADED: > LOG("Cancelling download of " + this.sourceURI.spec); > this.state = AddonManager.STATE_CANCELLED; > XPIProvider.removeActiveInstall(this); > AddonManagerPrivate.callInstallListeners("onDownloadCancelled", > this.listeners, this.wrapper); >- this.removeTemporaryFile(); >+ if (this.state != AddonManager.STATE_AVAILABLE) >+ this.removeTemporaryFile(this.file); When does this happen? > > // If a listener changed our state then do not proceed with the download > if (this.state != AddonManager.STATE_DOWNLOADING) > return; > > try { > this.file = getTemporaryFile(); > this.ownsTempFile = true; >+ if (this.stream) >+ this.stream.close(); How can this happen? > }, > > /** > * This is the first chance to get at real headers on the channel. > * > * @see nsIStreamListener > */ > onStartRequest: function AI_onStartRequest(aRequest, aContext) { >+ let file = aContext.QueryInterface(Ci.nsIFile); >+ // If this.file is replaced a new one, we have to do nothing here. >+ if (!this.file.equals(file)) { >+ return; >+ } How can we end up not tracking the file we're downloading? > onStopRequest: function AI_onStopRequest(aRequest, aContext, aStatus) { >- this.stream.close(); >- this.channel = null; >- this.badCerthandler = null; >- Services.obs.removeObserver(this, "network:offline-about-to-go-offline"); >+ if (this.channel == aRequest) { >+ this.stream.close(); >+ this.stream = null; >+ this.channel = null; >+ this.badCerthandler = null; >+ Services.obs.removeObserver(this, "network:offline-about-to-go-offline"); >+ } When would we receive progress events for some other channel?
Whiteboard: [orange] → [orange] fails in test_cancel_restart and test_localfile
(In reply to comment #198) > Comment on attachment 512670 [details] [diff] [review] > remove correct temporary file > > Not really following what the problem is that you're trying to fix here. > > > this.channel.cancel(Cr.NS_BINDING_ABORTED); > > case AddonManager.STATE_AVAILABLE: > > case AddonManager.STATE_DOWNLOADED: > > LOG("Cancelling download of " + this.sourceURI.spec); > > this.state = AddonManager.STATE_CANCELLED; > > XPIProvider.removeActiveInstall(this); > > AddonManagerPrivate.callInstallListeners("onDownloadCancelled", > > this.listeners, this.wrapper); > >- this.removeTemporaryFile(); > >+ if (this.state != AddonManager.STATE_AVAILABLE) > >+ this.removeTemporaryFile(this.file); > > When does this happen? If this.state equls STATE_AVAILABLE, the temporary file is not created yet. > > // If a listener changed our state then do not proceed with the download > > if (this.state != AddonManager.STATE_DOWNLOADING) > > return; > > > > try { > > this.file = getTemporaryFile(); > > this.ownsTempFile = true; > >+ if (this.stream) > >+ this.stream.close(); > > How can this happen? If downloading after cancel is restarted before onStopRequest() for the cancel is invoked. At that time, we have no chance to close the stream except here. > > }, > > > > /** > > * This is the first chance to get at real headers on the channel. > > * > > * @see nsIStreamListener > > */ > > onStartRequest: function AI_onStartRequest(aRequest, aContext) { > >+ let file = aContext.QueryInterface(Ci.nsIFile); > >+ // If this.file is replaced a new one, we have to do nothing here. > >+ if (!this.file.equals(file)) { > >+ return; > >+ } > > How can we end up not tracking the file we're downloading? I am sorry, I can not understand what you mean. Anyway my intention here is that we do not need to prepare for crypto or others for download if the download is already cancelled. > > onStopRequest: function AI_onStopRequest(aRequest, aContext, aStatus) { > >- this.stream.close(); > >- this.channel = null; > >- this.badCerthandler = null; > >- Services.obs.removeObserver(this, "network:offline-about-to-go-offline"); > >+ if (this.channel == aRequest) { > >+ this.stream.close(); > >+ this.stream = null; > >+ this.channel = null; > >+ this.badCerthandler = null; > >+ Services.obs.removeObserver(this, "network:offline-about-to-go-offline"); > >+ } > > When would we receive progress events for some other channel? Ah, I did not care about progress events. onDataAvailable is for the progress events, right? We have to check the events come from the current downloading (I mean it's not cancelled.) in onDataAvailable.
(In reply to comment #200) > (In reply to comment #198) > > Comment on attachment 512670 [details] [diff] [review] > > remove correct temporary file > > > > Not really following what the problem is that you're trying to fix here. > > > > > this.channel.cancel(Cr.NS_BINDING_ABORTED); > > > case AddonManager.STATE_AVAILABLE: > > > case AddonManager.STATE_DOWNLOADED: > > > LOG("Cancelling download of " + this.sourceURI.spec); > > > this.state = AddonManager.STATE_CANCELLED; > > > XPIProvider.removeActiveInstall(this); > > > AddonManagerPrivate.callInstallListeners("onDownloadCancelled", > > > this.listeners, this.wrapper); > > >- this.removeTemporaryFile(); > > >+ if (this.state != AddonManager.STATE_AVAILABLE) > > >+ this.removeTemporaryFile(this.file); > > > > When does this happen? > > If this.state equals STATE_AVAILABLE, the temporary file is not created yet. But this.state is set to STATE_CANCELLED just above, how can it ever go back to being STATE_AVAILABLE? > > > // If a listener changed our state then do not proceed with the download > > > if (this.state != AddonManager.STATE_DOWNLOADING) > > > return; > > > > > > try { > > > this.file = getTemporaryFile(); > > > this.ownsTempFile = true; > > >+ if (this.stream) > > >+ this.stream.close(); > > > > How can this happen? > > If downloading after cancel is restarted before onStopRequest() for the cancel > is invoked. At that time, we have no chance to close the stream except here. Restarting the download before onStopRequest has fired isn't supported yet (this is bug 611755). Is this what you're trying to solve? The test itself should already be working around that, see the comment in test_cancel_restart, is this not working? > > > }, > > > > > > /** > > > * This is the first chance to get at real headers on the channel. > > > * > > > * @see nsIStreamListener > > > */ > > > onStartRequest: function AI_onStartRequest(aRequest, aContext) { > > >+ let file = aContext.QueryInterface(Ci.nsIFile); > > >+ // If this.file is replaced a new one, we have to do nothing here. > > >+ if (!this.file.equals(file)) { > > >+ return; > > >+ } > > > > How can we end up not tracking the file we're downloading? > > I am sorry, I can not understand what you mean. Anyway my intention here is > that we do not need to prepare for crypto or others for download if the > download is already cancelled. The question was when would we ever be downloading to something other than this.file? If the download got cancelled and then gets restarted we do have to recreate the crypto object or it will still contain the data that got downloaded previously. > > > onStopRequest: function AI_onStopRequest(aRequest, aContext, aStatus) { > > >- this.stream.close(); > > >- this.channel = null; > > >- this.badCerthandler = null; > > >- Services.obs.removeObserver(this, "network:offline-about-to-go-offline"); > > >+ if (this.channel == aRequest) { > > >+ this.stream.close(); > > >+ this.stream = null; > > >+ this.channel = null; > > >+ this.badCerthandler = null; > > >+ Services.obs.removeObserver(this, "network:offline-about-to-go-offline"); > > >+ } > > > > When would we receive progress events for some other channel? > > Ah, I did not care about progress events. onDataAvailable is for the progress > events, right? We have to check the events come from the current downloading (I > mean it's not cancelled.) in onDataAvailable. onStopRequest is fired when cancelling the download and after that point we shouldn't hear anymore events from the old download.
(In reply to comment #201) > (In reply to comment #200) > > (In reply to comment #198) > > > Comment on attachment 512670 [details] [diff] [review] > > > remove correct temporary file > > > > > > Not really following what the problem is that you're trying to fix here. > > > > > > > this.channel.cancel(Cr.NS_BINDING_ABORTED); > > > > case AddonManager.STATE_AVAILABLE: > > > > case AddonManager.STATE_DOWNLOADED: > > > > LOG("Cancelling download of " + this.sourceURI.spec); > > > > this.state = AddonManager.STATE_CANCELLED; > > > > XPIProvider.removeActiveInstall(this); > > > > AddonManagerPrivate.callInstallListeners("onDownloadCancelled", > > > > this.listeners, this.wrapper); > > > >- this.removeTemporaryFile(); > > > >+ if (this.state != AddonManager.STATE_AVAILABLE) > > > >+ this.removeTemporaryFile(this.file); > > > > > > When does this happen? > > > > If this.state equals STATE_AVAILABLE, the temporary file is not created yet. > > But this.state is set to STATE_CANCELLED just above, how can it ever go back to > being STATE_AVAILABLE? Gosh! My mistake. Then the check is useless. > > > > // If a listener changed our state then do not proceed with the download > > > > if (this.state != AddonManager.STATE_DOWNLOADING) > > > > return; > > > > > > > > try { > > > > this.file = getTemporaryFile(); > > > > this.ownsTempFile = true; > > > >+ if (this.stream) > > > >+ this.stream.close(); > > > > > > How can this happen? > > > > If downloading after cancel is restarted before onStopRequest() for the cancel > > is invoked. At that time, we have no chance to close the stream except here. > > Restarting the download before onStopRequest has fired isn't supported yet > (this is bug 611755). Is this what you're trying to solve? The test itself > should already be working around that, see the comment in test_cancel_restart, > is this not working? I don't try to fix bug 611755. I think that bug is against onDownloadCancelled event. Actually I guess the fix for bug 611755 is easier than this. Something like the following: + let file = this.file; AddonManagerPrivate.callInstallListeners("onDownloadCancelled", this.listeners, this.wrapper); - this.removeTemporaryFile(); + this.removeTemporaryFile(file); And the executeSoon workaround does not work properly for this issue. (Actually I do not think the workaround is for this bug but the workaround is not needed if this patch is applied.) From the log in comment 193. TEST-INFO | chrome://mochitests/content/browser/browser/base/content/test/browser_bug553455.js | Install of http://example.com/browser/toolkit/mozapps/extensions/test/xpinstall/unsigned.xpi was in state 4 TEST-INFO | chrome://mochitests/content/browser/browser/base/content/test/browser_bug553455.js | Console message: LOG addons.xpi: Download started for http://example.com/browser/toolkit/mozapps/extensions/test/xpinstall/unsigned.xpi to file C:\DOCUME~1\cltbld\LOCALS~1\Temp\tmp-2uz.xpi TEST-INFO | chrome://mochitests/content/browser/browser/base/content/test/browser_bug553455.js | Console message: LOG addons.xpi: Download of http://example.com/browser/toolkit/mozapps/extensions/test/xpinstall/unsigned.xpi completed. TEST-INFO | chrome://mochitests/content/browser/browser/base/content/test/browser_bug553455.js | Console message: [JavaScript Warning: "WARN addons.xpi: Download failed: [Exception... "Component returned failure code: 0x80520012 (NS_ERROR_FILE_NOT_FOUND) [nsIZipReader.open]" nsresult: "0x80520012 (NS_ERROR_FILE_NOT_FOUND)" location: "JS frame :: resource://gre/modules/XPIProvider.jsm :: AI_loadManifest :: line 5360" data: no]" {file: "resource://gre/modules/XPIProvider.jsm" line: 5360}] TEST-INFO | chrome://mochitests/content/browser/browser/base/content/test/browser_bug553455.js | Longer timeout required, waiting longer... Remaining timeouts: 3 nsIZipReader.open returns NS_ERROR_FILE_NOT_FOUND because the target temporary file had been already removed when cancelling the download. So we have to remove the correct (cancelled) temporary file. > > > > }, > > > > > > > > /** > > > > * This is the first chance to get at real headers on the channel. > > > > * > > > > * @see nsIStreamListener > > > > */ > > > > onStartRequest: function AI_onStartRequest(aRequest, aContext) { > > > >+ let file = aContext.QueryInterface(Ci.nsIFile); > > > >+ // If this.file is replaced a new one, we have to do nothing here. > > > >+ if (!this.file.equals(file)) { > > > >+ return; > > > >+ } > > > > > > How can we end up not tracking the file we're downloading? > > > > I am sorry, I can not understand what you mean. Anyway my intention here is > > that we do not need to prepare for crypto or others for download if the > > download is already cancelled. > > The question was when would we ever be downloading to something other than > this.file? The problem is that downloading is always to "this.file" but onStart/onStop request is sometimes not for this.file due to asynchronous. > If the download got cancelled and then gets restarted we do have to recreate > the crypto object or it will still contain the data that got downloaded > previously. I think onStartRequest part of this fix is exactly what you are saying. If the check is not there, we also create the crypto against the cancelled download. > > > > onStopRequest: function AI_onStopRequest(aRequest, aContext, aStatus) { > > > >- this.stream.close(); > > > >- this.channel = null; > > > >- this.badCerthandler = null; > > > >- Services.obs.removeObserver(this, "network:offline-about-to-go-offline"); > > > >+ if (this.channel == aRequest) { > > > >+ this.stream.close(); > > > >+ this.stream = null; > > > >+ this.channel = null; > > > >+ this.badCerthandler = null; > > > >+ Services.obs.removeObserver(this, "network:offline-about-to-go-offline"); > > > >+ } > > > > > > When would we receive progress events for some other channel? > > > > Ah, I did not care about progress events. onDataAvailable is for the progress > > events, right? We have to check the events come from the current downloading (I > > mean it's not cancelled.) in onDataAvailable. > > onStopRequest is fired when cancelling the download and after that point we > shouldn't hear anymore events from the old download. Thanks. I will do so.
(In reply to comment #202) > And the executeSoon workaround does not work properly for this issue. (Actually > I do not think the workaround is for this bug but the workaround is not needed > if this patch is applied.) > > From the log in comment 193. > > TEST-INFO | > chrome://mochitests/content/browser/browser/base/content/test/browser_bug553455.js > | Install of > http://example.com/browser/toolkit/mozapps/extensions/test/xpinstall/unsigned.xpi > was in state 4 > TEST-INFO | > chrome://mochitests/content/browser/browser/base/content/test/browser_bug553455.js > | Console message: LOG addons.xpi: Download started for > http://example.com/browser/toolkit/mozapps/extensions/test/xpinstall/unsigned.xpi > to file C:\DOCUME~1\cltbld\LOCALS~1\Temp\tmp-2uz.xpi > TEST-INFO | > chrome://mochitests/content/browser/browser/base/content/test/browser_bug553455.js > | Console message: LOG addons.xpi: Download of > http://example.com/browser/toolkit/mozapps/extensions/test/xpinstall/unsigned.xpi > completed. > TEST-INFO | > chrome://mochitests/content/browser/browser/base/content/test/browser_bug553455.js > | Console message: [JavaScript Warning: "WARN addons.xpi: Download failed: > [Exception... "Component returned failure code: 0x80520012 > (NS_ERROR_FILE_NOT_FOUND) [nsIZipReader.open]" nsresult: "0x80520012 > (NS_ERROR_FILE_NOT_FOUND)" location: "JS frame :: > resource://gre/modules/XPIProvider.jsm :: AI_loadManifest :: line 5360" data: > no]" {file: "resource://gre/modules/XPIProvider.jsm" line: 5360}] > TEST-INFO | > chrome://mochitests/content/browser/browser/base/content/test/browser_bug553455.js > | Longer timeout required, waiting longer... Remaining timeouts: 3 > > nsIZipReader.open returns NS_ERROR_FILE_NOT_FOUND because the target temporary > file had been already removed when cancelling the download. So we have to > remove the correct (cancelled) temporary file. The temporary file should get removed for the cancelled download, then a new file should be there for the new download. > > > > > }, > > > > > > > > > > /** > > > > > * This is the first chance to get at real headers on the channel. > > > > > * > > > > > * @see nsIStreamListener > > > > > */ > > > > > onStartRequest: function AI_onStartRequest(aRequest, aContext) { > > > > >+ let file = aContext.QueryInterface(Ci.nsIFile); > > > > >+ // If this.file is replaced a new one, we have to do nothing here. > > > > >+ if (!this.file.equals(file)) { > > > > >+ return; > > > > >+ } > > > > > > > > How can we end up not tracking the file we're downloading? > > > > > > I am sorry, I can not understand what you mean. Anyway my intention here is > > > that we do not need to prepare for crypto or others for download if the > > > download is already cancelled. > > > > The question was when would we ever be downloading to something other than > > this.file? > > The problem is that downloading is always to "this.file" but onStart/onStop > request is sometimes not for this.file due to asynchronous. I still don't understand how that can happen unless somehow we are restarting the download before onStopRequest has been called. If we are then we need to fix that (and probably add guards to ensure we don't do it elsewhere). > > If the download got cancelled and then gets restarted we do have to recreate > > the crypto object or it will still contain the data that got downloaded > > previously. > > I think onStartRequest part of this fix is exactly what you are saying. If the > check is not there, we also create the crypto against the cancelled download. Well it has to be created for the cancelled download when that download starts because we don't know that we'll cancel it. It then has to be re-created for the next download. It has to be created for every onStartRequest we see.
Comment on attachment 512670 [details] [diff] [review] remove correct temporary file Based on the discussion so far I still don't fully understand what this is solving and there are certainly some changes to be made so minusing for now.
Attachment #512670 - Flags: review?(dtownsend) → review-
(In reply to comment #203) > (In reply to comment #202) > > And the executeSoon workaround does not work properly for this issue. (Actually > > I do not think the workaround is for this bug but the workaround is not needed > > if this patch is applied.) > > > > From the log in comment 193. > > > > TEST-INFO | > > chrome://mochitests/content/browser/browser/base/content/test/browser_bug553455.js > > | Install of > > http://example.com/browser/toolkit/mozapps/extensions/test/xpinstall/unsigned.xpi > > was in state 4 > > TEST-INFO | > > chrome://mochitests/content/browser/browser/base/content/test/browser_bug553455.js > > | Console message: LOG addons.xpi: Download started for > > http://example.com/browser/toolkit/mozapps/extensions/test/xpinstall/unsigned.xpi > > to file C:\DOCUME~1\cltbld\LOCALS~1\Temp\tmp-2uz.xpi > > TEST-INFO | > > chrome://mochitests/content/browser/browser/base/content/test/browser_bug553455.js > > | Console message: LOG addons.xpi: Download of > > http://example.com/browser/toolkit/mozapps/extensions/test/xpinstall/unsigned.xpi > > completed. > > TEST-INFO | > > chrome://mochitests/content/browser/browser/base/content/test/browser_bug553455.js > > | Console message: [JavaScript Warning: "WARN addons.xpi: Download failed: > > [Exception... "Component returned failure code: 0x80520012 > > (NS_ERROR_FILE_NOT_FOUND) [nsIZipReader.open]" nsresult: "0x80520012 > > (NS_ERROR_FILE_NOT_FOUND)" location: "JS frame :: > > resource://gre/modules/XPIProvider.jsm :: AI_loadManifest :: line 5360" data: > > no]" {file: "resource://gre/modules/XPIProvider.jsm" line: 5360}] > > TEST-INFO | > > chrome://mochitests/content/browser/browser/base/content/test/browser_bug553455.js > > | Longer timeout required, waiting longer... Remaining timeouts: 3 > > > > nsIZipReader.open returns NS_ERROR_FILE_NOT_FOUND because the target temporary > > file had been already removed when cancelling the download. So we have to > > remove the correct (cancelled) temporary file. > > The temporary file should get removed for the cancelled download, then a new > file should be there for the new download. Yes, that is what I am trying to fix. > > > > > > }, > > > > > > > > > > > > /** > > > > > > * This is the first chance to get at real headers on the channel. > > > > > > * > > > > > > * @see nsIStreamListener > > > > > > */ > > > > > > onStartRequest: function AI_onStartRequest(aRequest, aContext) { > > > > > >+ let file = aContext.QueryInterface(Ci.nsIFile); > > > > > >+ // If this.file is replaced a new one, we have to do nothing here. > > > > > >+ if (!this.file.equals(file)) { > > > > > >+ return; > > > > > >+ } > > > > > > > > > > How can we end up not tracking the file we're downloading? > > > > > > > > I am sorry, I can not understand what you mean. Anyway my intention here is > > > > that we do not need to prepare for crypto or others for download if the > > > > download is already cancelled. > > > > > > The question was when would we ever be downloading to something other than > > > this.file? > > > > The problem is that downloading is always to "this.file" but onStart/onStop > > request is sometimes not for this.file due to asynchronous. > > I still don't understand how that can happen unless somehow we are restarting > the download before onStopRequest has been called. I do not think that situation happens unless restart the download. > > > If the download got cancelled and then gets restarted we do have to recreate > > > the crypto object or it will still contain the data that got downloaded > > > previously. > > > > I think onStartRequest part of this fix is exactly what you are saying. If the > > check is not there, we also create the crypto against the cancelled download. > > Well it has to be created for the cancelled download when that download starts > because we don't know that we'll cancel it. It then has to be re-created for > the next download. It has to be created for every onStartRequest we see. But if this.file does not equal file which is accompany with onStartRequest, the download to the file is already cancelled. Right? We have to create the crypt even if the download is already cancelled? To be more clear what I want to do @@ -5500,17 +5503,18 @@ AddonInstall.prototype = { Components.utils.import("resource://gre/modules/CertUtils.jsm"); let requireBuiltIn = Prefs.getBoolPref(PREF_INSTALL_REQUIREBUILTINCERTS, true); this.badCertHandler = new BadCertHandler(!requireBuiltIn); this.channel = NetUtil.newChannel(this.sourceURI); this.channel.notificationCallbacks = this; if (this.channel instanceof Ci.nsIHttpChannelInternal) this.channel.forceAllowThirdPartyCookie = true; - this.channel.asyncOpen(listener, null); + var file = this.file; + this.channel.asyncOpen(listener, file); The file object which is passed to asyncOpen is preserved even if restart downloading (this.file is replaced new temporary file).
(In reply to comment #205) > > > > > > > }, > > > > > > > > > > > > > > /** > > > > > > > * This is the first chance to get at real headers on the channel. > > > > > > > * > > > > > > > * @see nsIStreamListener > > > > > > > */ > > > > > > > onStartRequest: function AI_onStartRequest(aRequest, aContext) { > > > > > > >+ let file = aContext.QueryInterface(Ci.nsIFile); > > > > > > >+ // If this.file is replaced a new one, we have to do nothing here. > > > > > > >+ if (!this.file.equals(file)) { > > > > > > >+ return; > > > > > > >+ } > > > > > > > > > > > > How can we end up not tracking the file we're downloading? > > > > > > > > > > I am sorry, I can not understand what you mean. Anyway my intention here is > > > > > that we do not need to prepare for crypto or others for download if the > > > > > download is already cancelled. > > > > > > > > The question was when would we ever be downloading to something other than > > > > this.file? > > > > > > The problem is that downloading is always to "this.file" but onStart/onStop > > > request is sometimes not for this.file due to asynchronous. > > > > I still don't understand how that can happen unless somehow we are restarting > > the download before onStopRequest has been called. > > I do not think that situation happens unless restart the download. Are you saying that we are restarting the download before onStopRequest has been called for the cancelled download? > > > > If the download got cancelled and then gets restarted we do have to recreate > > > > the crypto object or it will still contain the data that got downloaded > > > > previously. > > > > > > I think onStartRequest part of this fix is exactly what you are saying. If the > > > check is not there, we also create the crypto against the cancelled download. > > > > Well it has to be created for the cancelled download when that download starts > > because we don't know that we'll cancel it. It then has to be re-created for > > the next download. It has to be created for every onStartRequest we see. > > But if this.file does not equal file which is accompany with onStartRequest, > the download to the file is already cancelled. Right? We have to create the > crypt even if the download is already cancelled? I don't believe it is physically possible to cancel the download before onStartRequest is called is it? Even if it is we shouldn't be allowing the next download to start until the onStopRequest has been called in which case we don't need to worry about onStartRequest looking at the right file, it always will be. All of this said I'd be extremely unlikely to consider changing the code in XPIProvider.jsm unless there is a glaring problem that a user could trigger manually and has an extremely simple fix. If possible wallpapering over it in the test is the safe way to go at this stage of the release.
(In reply to comment #206) > (In reply to comment #205) > > > > > > > > }, > > > > > > > > > > > > > > > > /** > > > > > > > > * This is the first chance to get at real headers on the channel. > > > > > > > > * > > > > > > > > * @see nsIStreamListener > > > > > > > > */ > > > > > > > > onStartRequest: function AI_onStartRequest(aRequest, aContext) { > > > > > > > >+ let file = aContext.QueryInterface(Ci.nsIFile); > > > > > > > >+ // If this.file is replaced a new one, we have to do nothing here. > > > > > > > >+ if (!this.file.equals(file)) { > > > > > > > >+ return; > > > > > > > >+ } > > > > > > > > > > > > > > How can we end up not tracking the file we're downloading? > > > > > > > > > > > > I am sorry, I can not understand what you mean. Anyway my intention here is > > > > > > that we do not need to prepare for crypto or others for download if the > > > > > > download is already cancelled. > > > > > > > > > > The question was when would we ever be downloading to something other than > > > > > this.file? > > > > > > > > The problem is that downloading is always to "this.file" but onStart/onStop > > > > request is sometimes not for this.file due to asynchronous. > > > > > > I still don't understand how that can happen unless somehow we are restarting > > > the download before onStopRequest has been called. > > > > I do not think that situation happens unless restart the download. > > Are you saying that we are restarting the download before onStopRequest has > been called for the cancelled download? Right. At least the failure in test_cancel_restart case. > > > > > If the download got cancelled and then gets restarted we do have to recreate > > > > > the crypto object or it will still contain the data that got downloaded > > > > > previously. > > > > > > > > I think onStartRequest part of this fix is exactly what you are saying. If the > > > > check is not there, we also create the crypto against the cancelled download. > > > > > > Well it has to be created for the cancelled download when that download starts > > > because we don't know that we'll cancel it. It then has to be re-created for > > > the next download. It has to be created for every onStartRequest we see. > > > > But if this.file does not equal file which is accompany with onStartRequest, > > the download to the file is already cancelled. Right? We have to create the > > crypt even if the download is already cancelled? > > I don't believe it is physically possible to cancel the download before > onStartRequest is called is it? Even if it is we shouldn't be allowing the next > download to start until the onStopRequest has been called in which case we > don't need to worry about onStartRequest looking at the right file, it always > will be. > > All of this said I'd be extremely unlikely to consider changing the code in > XPIProvider.jsm unless there is a glaring problem that a user could trigger > manually and has an extremely simple fix. If possible wallpapering over it in > the test is the safe way to go at this stage of the release. I do not say that "Please fix this issue before Firefox4 landing". But I can say this patch makes XPIProvider.jsm robust.
(In reply to comment #207) > (In reply to comment #206) > > All of this said I'd be extremely unlikely to consider changing the code in > > XPIProvider.jsm unless there is a glaring problem that a user could trigger > > manually and has an extremely simple fix. If possible wallpapering over it in > > the test is the safe way to go at this stage of the release. > > I do not say that "Please fix this issue before Firefox4 landing". But I can > say this patch makes XPIProvider.jsm robust. Sure but for now I'd much rather focus on getting the test stable for when we branch. If you're not interested in working on that then that's fine, I have some time available now to look into it.
(In reply to comment #210) > (In reply to comment #207) > > (In reply to comment #206) > > > All of this said I'd be extremely unlikely to consider changing the code in > > > XPIProvider.jsm unless there is a glaring problem that a user could trigger > > > manually and has an extremely simple fix. If possible wallpapering over it in > > > the test is the safe way to go at this stage of the release. > > > > I do not say that "Please fix this issue before Firefox4 landing". But I can > > say this patch makes XPIProvider.jsm robust. > > Sure but for now I'd much rather focus on getting the test stable for when we > branch. If you're not interested in working on that then that's fine, I have > some time available now to look into it. Ah I am sorry that I misunderstood what you are saying. You mean workaround is needed in the test code, we should not fix XPIProvider.jsm if the fix is simple, right? attachment 512670 [details] [diff] [review] is simple to me anyway.
(In reply to comment #211) > (In reply to comment #210) > > (In reply to comment #207) > > > (In reply to comment #206) > > > > All of this said I'd be extremely unlikely to consider changing the code in > > > > XPIProvider.jsm unless there is a glaring problem that a user could trigger > > > > manually and has an extremely simple fix. If possible wallpapering over it in > > > > the test is the safe way to go at this stage of the release. > > > > > > I do not say that "Please fix this issue before Firefox4 landing". But I can > > > say this patch makes XPIProvider.jsm robust. > > > > Sure but for now I'd much rather focus on getting the test stable for when we > > branch. If you're not interested in working on that then that's fine, I have > > some time available now to look into it. > > Ah I am sorry that I misunderstood what you are saying. You mean workaround is > needed in the test code, we should not fix XPIProvider.jsm if the fix is > simple, right? Oops! "if the fix is NOT simple".
Comment on attachment 512693 [details] [diff] [review] Fix havoc of browser_bug592338.js [checked-in] Landed this: http://hg.mozilla.org/mozilla-central/rev/b99674516b45
Attachment #512693 - Attachment description: Fix havoc of browser_bug592338.js → Fix havoc of browser_bug592338.js [checked-in]
Bug 623242 may have resolved the timeout issue in test_localfile, any failures after this point would indicate otherwise
(In reply to comment #218) > Bug 623242 may have resolved the timeout issue in test_localfile, any failures > after this point would indicate otherwise Wow! Awesome! You did it!!
I managed to reproduce the issue from comment 207, that of onStopRequest not getting called until after we attempt to restart the download which means we are falling into the problem of bug 611755. Apparently the executeSoon is not enough on its own to avoid this all the time. There is a patch there that probably solves this but we don't want to take it for Firefox 4 so instead here is workaround for the test. This code detects when the http request is created and substitutes our own listener that just passes everything through to the original listener but allows us to only restart the download after onStopRequest has completed. It looks like a lot of change but most of it is whitespace and moving.
Attachment #516119 - Flags: review?(gavin.sharp)
Attachment #516119 - Flags: review?(gavin.sharp) → review+
Keywords: checkin-needed
Landed: http://hg.mozilla.org/mozilla-central/rev/475ae5b49540
Status: NEW → RESOLVED
Closed: 15 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Dave, what exactly does this patch fix? Looks like we still see crashes for this test.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Whiteboard: [orange] fails in test_cancel_restart and test_localfile → [orange] fails in test_cancel_restart
Attachment #516119 - Attachment description: wait for onStopRequest → wait for onStopRequest [checked-in]
I could reproduce this on local machine and found cancel button is pressed before starting HTTP transaction for unsigned xpi. I hope this patch fixes this issue.
Attachment #527480 - Flags: review?(dtownsend)
Comment on attachment 516119 [details] [diff] [review] wait for onStopRequest [backed-out] I've backed this out in http://hg.mozilla.org/mozilla-central/rev/edd299ad7142 since bug 611755 should make it obsolete. Going to see whether this helps, if not may need to re-land this and review the new patch here.
Attachment #516119 - Attachment description: wait for onStopRequest [checked-in] → wait for onStopRequest [backed-out]
Thank last failure was in test_blocked_install again, can't see how the changes I landed could have had an impact to that part of the test though.
Whiteboard: [orange] fails in test_cancel_restart → [orange] fails in test_cancel_restart and test_blocked_install
Attachment #527480 - Flags: review?(dtownsend)
(In reply to comment #331) > Comment on attachment 516119 [details] [diff] [review] > wait for onStopRequest [backed-out] > > I've backed this out in http://hg.mozilla.org/mozilla-central/rev/edd299ad7142 > since bug 611755 should make it obsolete. Going to see whether this helps, if > not may need to re-land this and review the new patch here. Good to hear. That test code actually was little tricky. But unfortunately the fix for bug 611755 seems not be fixed the timeout of test_cancel_restart.
(In reply to comment #336) > (In reply to comment #331) > > Comment on attachment 516119 [details] [diff] [review] > > wait for onStopRequest [backed-out] > > > > I've backed this out in http://hg.mozilla.org/mozilla-central/rev/edd299ad7142 > > since bug 611755 should make it obsolete. Going to see whether this helps, if > > not may need to re-land this and review the new patch here. > > Good to hear. That test code actually was little tricky. > > But unfortunately the fix for bug 611755 seems not be fixed the timeout of > test_cancel_restart. Too early to say, there hasn't been one since it landed. Comment 333 was in test_blocked_install and comment 335 was a run from before the patch landed.
That last test failure was a try run based on a m-c changeset from the day before the patch landed (127b9421c7c3)
Then let's call this fixed, and reopen if it happens again. Thanks guys!
Target Milestone: --- → mozilla6
Blocks: 648353
Status: REOPENED → RESOLVED
Closed: 15 years ago → 15 years ago
Resolution: --- → FIXED
Note if this actually happens again I think we should file a new bug, this one has morphed through so many different cases that I don't think its history is worth much
Whiteboard: [orange] fails in test_cancel_restart and test_blocked_install → [orange]
Depends on: 664923
Whiteboard: [orange]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: