545734 - need to hide the iframe used for submitting plugin crash reports

Status: RESOLVED FIXED

(Toolkit :: Crash Reporting, defect)

Description

[Justin Dolske [:Dolske]]

Got a couple reports of a mysterious box appearing in the browser UI, containing text with a crashID... "there was a frame on the right, with a width of about 150-200px, white apart a crash ID into it. it was starting under bookmarks toolbar and going down till the statusbar. tabbar was on the left of this frame that was not closeable nor resizeable nor whatever."

I think what's happening is that the iframe added by CrashSubmit.jsm needs to hidden (display:none or make is 0x0). I don't see that being done anywhere. Mak also noted that this iframe seems to persist... Not sure if it was just really really slow to submit, or there's a case where it fails to get removed (looks like the code does at least attempt to remove the iframe after submission).

Comment 1

[Blair McBride [:Unfocused] (UNAVAILABLE)]

I don't think it's just a slow submission - I had it appear yesterday, and it persisted overnight. Unless it's a timeout, i guess.

Comment 2

[(not currently active) Ted Mielczarek]

Bleh, it ought to be removed, dunno why it wouldn't be. Also, I wish we had proper APIs so this didn't have to use an iframe.

Comment 3

[Reed Loden [:reed]]

Attachment: attack of the iframes

This is what one of my windows looks like right now, thanks to this bug and the #1 topcrash bug on Linux.

Comment 4

[(not currently active) Ted Mielczarek]

Heh. I wonder if the CrashSubmit code is failing here because it expects to be operating on HTML? about:crashes is XHTML, but clearly when used in browser.xul it's operating on XUL.

Assuming the webprogresslistener gets all the way to STATE_STOP, it should remove the iframe no matter what:
http://mxr.mozilla.org/mozilla-central/source/toolkit/crashreporter/CrashSubmit.jsm#268

Might be interesting to stick some extra debug statements in there to find out what's happening.

Comment 5

[Justin Dolske [:Dolske]]

Is anyone seeing this on Windows?

Comment 6

[Benjamin Smedberg]

Error: this.iframe.docShell.removeProgressListener is not a function
Source File: file:///builds/minefield-x86/firefox/modules/CrashSubmit.jsm
Line: 269

This also seemingly prevents the plugin-crash UI from actually showing.

Comment 7

[Benjamin Smedberg]

Note, #testday participants also say that whenever the crash reporter is enabled on Linux they never see the plugin-crashed UI. The iframe being stuck visible is only an occasional problem beyond that.

Comment 8

[Benjamin Smedberg]

more details: the first time I go to http://flashcrash.dempsky.org/ the iframe doesn't appear, but no plugin-crash UI. I hit refresh, and the iframe appears with the error console message about .removeProgressListener.

Comment 9

[(not currently active) Ted Mielczarek]

The code explicitly creates a xul:iframe:
http://mxr.mozilla.org/mozilla-central/source/toolkit/crashreporter/CrashSubmit.jsm#312

I don't know why that would fail.

Comment 10

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to comment #5)
> Is anyone seeing this on Windows?

Yes, Windows 7 here.

Comment 11

[Justin Dolske [:Dolske]]

I suspect some (all?) cases of the plugin-crashed UI not being shown are either bug 544550 or bug 545686.

Comment 12

[Justin Dolske [:Dolske]]

Attachment: Patch v.1

I was able to reproduce the sticky iframe by unplugging my network cable, the form submission fails after a few seconds and the frame ends up holding an error page. [Note to self: also need to set MOZ_CRASHREPORTER=1 to trigger submission in own builds.]

I got other errors if I styled the iframe with display:none (presumably that's bad to do for an iframe with content you're trying to manipulate?). So, instead, I set width=0 plus a minWidth override for the default iframe style.

The error that was being thrown in some cases is fixed by adding a QI. I guess the error page results in a new docshell (which hasn't been QI'd to this interface yet?)...

Comment 13

[(not currently active) Ted Mielczarek]

Comment on attachment 427047 [details] [diff] [review]
Patch v.1

>diff --git a/toolkit/crashreporter/CrashSubmit.jsm b/toolkit/crashreporter/CrashSubmit.jsm
>--- a/toolkit/crashreporter/CrashSubmit.jsm
>+++ b/toolkit/crashreporter/CrashSubmit.jsm
>@@ -261,16 +261,17 @@ Submitter.prototype = {
>         aIID.equals(Ci.nsISupports))
>       return this;
>     throw Components.results.NS_NOINTERFACE;
>   },
> 
>   onStateChange: function(aWebProgress, aRequest, aFlag, aStatus)
>   {
>     if(aFlag & STATE_STOP) {
>+      this.iframe.docShell.QueryInterface(Ci.nsIWebProgress);
>       this.iframe.docShell.removeProgressListener(this);
> 
>       // check general request status first
>       if (!Components.isSuccessCode(aStatus)) {
>         this.element.removeChild(this.iframe);
>         if (this.errorCallback) {
>           this.errorCallback(this.id);
>         }
>@@ -306,16 +307,18 @@ Submitter.prototype = {
>     if (!dump.exists() || !extra.exists()) {
>       this.cleanup();
>       return false;
>     }
>     this.dump = dump;
>     this.extra = extra;
>     let iframe = this.document.createElementNS("http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul", "iframe");
>     iframe.setAttribute("type", "content");
>+    iframe.style.width = 0;
>+    iframe.style.minWidth= 0;

nit: spaces around equals sign.

Looks fine otherwise, r=me

Comment 14

[Justin Dolske [:Dolske]]

Attachment: Patch v.2

Comment 15

[Justin Dolske [:Dolske]]

Pushed http://hg.mozilla.org/mozilla-central/rev/47994c0cb561

Comment 16

[Benjamin Smedberg]

http://hg.mozilla.org/projects/firefox-lorentz/rev/87e9e5cde35d

Comment 17

[Mike Beltzner [:beltzner, not reading bugmail]]

Blanket approval for Lorentz merge to mozilla-1.9.2
a=beltzner for 1.9.2.4 - please make sure to mark status1.9.2:.4-fixed

Comment 18

[Benjamin Smedberg]

Merged into 1.9.2 at http://hg.mozilla.org/releases/mozilla-1.9.2/rev/84ba4d805430