676719 - Record URLs for plugin processes

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect, P1)

Description

[Marcia Knous [:marcia]]

As we continue doing more investigation in the plugin realm, would be helpful to have URLs for these types of crashes. We are getting asked in bugs such as Bug 673433, for example.

Comment 1

[(not currently active) Ted Mielczarek]

This is a client-side bug. We'd need to add some code to the OOPP implementation to record this data.

Comment 2

[Benjamin Smedberg]

There's a bug on this with discussion already...

Comment 3

[Robert Kaiser]

No matter if in this bug or elsewhere, we really need to get something implemented for this, as from what I see the content crashes have the same problem, and we both want to be able to communicate to plugin vendors what URLs their problems happen on as well as find out what websites crash when using content process.

Comment 4

[Marcia Knous [:marcia]]

I couldn't find it in the list of bugs, so please feel free to dupe this bug to that bug and KaiRo and I will add ourselves to that bug.

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #2)
> There's a bug on this with discussion already...

Comment 5

[Sheila Mooney]

We are being asked more and more for URLs to help us diagnose some of the harder crash issues. The bugs just end up sitting there - a number of our top Flash issues are like that. Per KaiRo's comments, won't we need this for the content crashes also? I know this is probably a difficult problem to solve.

Any way we can find the other bug or if the discussion happened elsewhere, get it pasted in this bug so we are all up to speed on the issues?

Comment 6

[Robert Kaiser]

(In reply to Sheila Mooney from comment #5)
> Per KaiRo's comments, won't we need this for
> the content crashes also?

Apparently, we have some URLs for some content crashes. There seems to be a different problem at work why Fennec gets almost no URLs reported.

Still, we really need this more and more for plugins as we intensify our communications with plugin vendors and even vendors of applications running in plugins (think Farmville etc.) and so we really want something to move forward here.

Comment 7

[Drew Willcoxon :adw]

Benjamin, could you point me to the back-end code related to plugin crash-handling?  I know in the front end we capture crash events to do things like prompting the user to send a crash report, and I presume the report could be annotated at that point, but this bug should probably be fixed in the back end, right?

Comment 8

[Benjamin Smedberg]

Backend code: http://hg.mozilla.org/mozilla-central/annotate/d042ad078f43/dom/plugins/ipc/PluginModuleParent.cpp#l446

But I really don't think that doing this in the backend would be either easier or better. The frontend knows which tab was focused at the time the crash occurs, which is probably the tab that "caused" the crash (not for certain, but close enough for stats work!). So I think doing this in the frontend is very reasonable.

Comment 9

[Drew Willcoxon :adw]

Attachment: patch

Thanks, Benjamin.  This adds a PluginContentURL key to the form data sent to the server.

Comment 10

[Benjamin Smedberg]

Comment on attachment 666054 [details] [diff] [review]
patch

Looks good to me, but isn't CrashSubmit.jsm used from other places? Are you prepared to handle an "undefined" extraExtraKeyVals? ted should look this over. And it really needs a test. Since this is Firefox-specific chrome, we might need to add a test in a new location (the current locations in dom/plugins/test are generic).

Comment 11

[Benjamin Smedberg]

Having pondered this, I think we are missing an important piece of UI: URLs have an opt-out checkbox, and this patch would submit them automatically without the user being explicitly aware of it. That's why I lumped this bug and bug 648675 together, since they both require UI changes and it may make sense to have a completely different UI (e.g. a popout).

Comment 12

[Drew Willcoxon :adw]

Gavin thinks a checkbox would be OK.  I'll put it under the comment box and above the Send button in Jennifer's mockup in bug 648675 and do both bugs at the same time.

Comment 13

[Drew Willcoxon :adw]

Comment on attachment 666054 [details] [diff] [review]
patch

The patch in bug 648675 covers this bug, too.

Comment 14

[Robert Kaiser]

Is that patch going to add UI so people can opt out of reporting the URl while still reporting the crash, or so they are at least informed that when they send the report, we include the URL? We need to do that out of privacy reasons, as far as I'm informed (and we do that for browser crashes - we have a default-unchecked checkbox there for reporting the URL9.

Comment 15

[(not currently active) Ted Mielczarek]

(It's default-checked, FWIW, but we persist the user's choice.)

Comment 16

[Drew Willcoxon :adw]

Comment on attachment 666054 [details] [diff] [review]
patch

Thought I marked this obsolete.

Comment 17

[Drew Willcoxon :adw]

This is fixed by bug 648675.