Closed Bug 1185042 Opened 5 years ago Closed 4 years ago

Sending message that cannot be cloned. Are you trying to send an XPCOM object? - Plugin crash report submission

Categories

(Core :: Plug-ins, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla42
Tracking Status
e10s + ---
firefox42 --- fixed

People

(Reporter: rowbot, Assigned: rowbot)

Details

Attachments

(1 file)

This message is showing up when running the browser_pluginCrashCommentAndURL[1] test.  See Bug 1160780, comment 376.  While working on that test, I narrowed the error down to occurring when the user submits the plugin crash report.  I'm guess that whatever message gets sent on plugin crash report submission must be sending something in the wrong format.  If this is the same issue as Bug 1156065, then its probably the page URL of the crashing plugin.

INFO Console message: [JavaScript Warning: "Sending message that cannot be cloned. Are you trying to send an XPCOM object?" {file: "resource:///modules/Plugi
nContent.jsm" line: 591}]

[1] https://dxr.mozilla.org/mozilla-central/source/browser/base/content/test/plugins/browser_pluginCrashCommentAndURL.js
mconley, I think you ported this machinery right?
tracking-e10s: --- → ?
Flags: needinfo?(mconley)
Yeah, that's me. Looking.
Flags: needinfo?(mconley)
Ah, heh, found it:

 submitReport: function submitReport(plugin) {
    if (!AppConstants.MOZ_CRASHREPORTER) {
      return;
    }
    if (!plugin) {
      Cu.reportError("Attempted to submit crash report without an associated plugin.");
      return;
    }
    if (!(plugin instanceof Ci.nsIObjectLoadingContent)) {
      Cu.reportError("Attempted to submit crash report on plugin that does not" +
                     "implement nsIObjectLoadingContent.");
      return;
    }

    let runID = plugin.runID;
    let submitURLOptIn = this.getPluginUI(plugin, "submitURLOptIn");
    let keyVals = {};
    let userComment = this.getPluginUI(plugin, "submitComment").value.trim();
    if (userComment)
      keyVals.PluginUserComment = userComment;
    if (this.getPluginUI(plugin, "submitURLOptIn").checked)
      keyVals.PluginContentURL = plugin.ownerDocument.URL;

    this.global.sendAsyncMessage("PluginContent:SubmitReport",
                                 { runID, keyVals, submitURLOptIn });
    // THIS IS A DOM NODE ----------------------------^
  },


Whoops
Trevor - feel like taking this one? It's easy-peasy, probably worth an uplift, and a fast review.
Flags: needinfo?(smokey101stair)
Sure, I'll post a patch for this in a bit.
Flags: needinfo?(smokey101stair)
Just checked - so we still pay attention to the user's wishes to not include the page URL, despite sending up the submitURLOptIn as a truthy DOM node to the parent. The only problem is that we don't persist this user preference to Preferences, so the check box is always checked for "Include the page's URL" (that's the only reason why we send that value to the parent).

Phew.
Hopefully, the plugin isn't crashing so often that they notice the option doesn't persist :).  While I was here, I got rid of the duplicate query for the checked state and replaced it with the cached one.
Assignee: nobody → smokey101stair
Attachment #8635475 - Flags: review?(mconley)
Comment on attachment 8635475 [details] [diff] [review]
bug1185042_do_not_send_dom_nodes.diff

Review of attachment 8635475 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good!
Attachment #8635475 - Flags: review?(mconley) → review+
Keywords: checkin-needed
Ah, I forgot to post a try push - I'll do that now.
Note that it appears as if the bug number is wrong in that commit message - we'll need to correct that before landing.
Ah, yes it is.  I'll drop the checkin-needed flag for the time being and fix the commit message when try comes back green.  Doing a try run totally slipped my mind before setting checkin-needed anyways.
Keywords: checkin-needed
Try is looking pretty green!
Flags: needinfo?(smokey101stair)
url:        https://hg.mozilla.org/integration/fx-team/rev/c6f1a0e2de42a91236b04f46fa3c589b6f0b3956
changeset:  c6f1a0e2de42a91236b04f46fa3c589b6f0b3956
user:       Trevor Rowbotham <smokey101stair@gmail.com>
date:       Fri Jul 17 12:16:00 2015 -0400
description:
Bug 1185042 - Correctly send the checked status of the URL opt-in checkbox when submitting a crash report. r=mconley
Flags: needinfo?(smokey101stair)
So this patch has changed things so that the "submit the URL" checkbox state is set when the user changes it - for example, the user has a plugin crash, and then in the UI, the user chooses to not submit the URL. In subsequent plugin crashes, the checkbox will auto-populate to the "unchecked" state, unless the user chooses to check it - after which, it'll auto-populate to the "checked" state.

This is different from what we do on Release, where it looks like we always read a user preference, "dom.ipc.plugins.reportCrashURL", to determine what to populate the checkbox with.

I assumed we wanted to do the former based on the way the code was laid out, since it looked like we _wanted_ that choice to be persisted to prefs... but I wanted to make sure this was known explicitly before I consider requesting uplift for this.

bsmedberg - does it sound right that we're persisting the user's choice here from crash to crash?
Flags: needinfo?(benjamin)
https://hg.mozilla.org/mozilla-central/rev/c6f1a0e2de42
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
Yes, I think we should be persisting this value.
Flags: needinfo?(benjamin)
You need to log in before you can comment on or make changes to this bug.