Intermittent failure in browser_bug553455.js | Test timed out

RESOLVED FIXED in mozilla6

Status

()

defect
RESOLVED FIXED
8 years ago
7 years ago

People

(Reporter: mounir, Unassigned)

Tracking

(Blocks 1 bug, {intermittent-failure})

Trunk
mozilla6
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments)

Reporter

Description

8 years ago
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
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
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?
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Reporter

Updated

8 years ago
Attachment #506095 - Flags: review? → review?(dtownsend)
Comment hidden (Legacy TBPL/Treeherder Robot)
Attachment #506095 - Flags: review?(dtownsend) → review+
Reporter

Updated

8 years ago
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]
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
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)
Comment hidden (Legacy TBPL/Treeherder Robot)
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
Comment hidden (Legacy TBPL/Treeherder Robot)
(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.
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
(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 hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
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]
Comment hidden (Legacy TBPL/Treeherder Robot)