need to hide the iframe used for submitting plugin crash reports

RESOLVED FIXED in mozilla1.9.3a2

Status

()

Toolkit
Crash Reporting
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: Dolske, Assigned: Dolske)

Tracking

({dogfood})

Trunk
mozilla1.9.3a2
dogfood
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(status1.9.2 .4-fixed)

Details

(Whiteboard: [fixed-lorentz])

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

8 years ago
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).
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.
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.
Keywords: dogfood
Created attachment 426642 [details]
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.
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.
(Assignee)

Comment 5

8 years ago
Is anyone seeing this on Windows?

Comment 6

8 years ago
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

8 years ago
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

8 years ago
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.
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.
(In reply to comment #5)
> Is anyone seeing this on Windows?

Yes, Windows 7 here.
(Assignee)

Comment 11

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

Comment 12

8 years ago
Created attachment 427047 [details] [diff] [review]
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?)...
Assignee: nobody → dolske
Attachment #426642 - Attachment is obsolete: true
Attachment #427047 - Flags: review?(ted.mielczarek)

Updated

8 years ago
Blocks: 545893
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
Attachment #427047 - Flags: review?(ted.mielczarek) → review+
(Assignee)

Comment 14

8 years ago
Created attachment 428783 [details] [diff] [review]
Patch v.2
Attachment #427047 - Attachment is obsolete: true
(Assignee)

Comment 15

8 years ago
Pushed http://hg.mozilla.org/mozilla-central/rev/47994c0cb561
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a2

Comment 16

8 years ago
http://hg.mozilla.org/projects/firefox-lorentz/rev/87e9e5cde35d
Whiteboard: [fixed-lorentz]
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

8 years ago
Merged into 1.9.2 at http://hg.mozilla.org/releases/mozilla-1.9.2/rev/84ba4d805430
status1.9.2: --- → .4-fixed
You need to log in before you can comment on or make changes to this bug.