Closed Bug 1139718 Opened 7 years ago Closed 7 years ago

Structured clone of Set and Regexp doesn't work via sendAsyncMessage if XPCOM objects are also sent

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla40
Tracking Status
e10s m7+ ---
firefox40 --- fixed

People

(Reporter: MattN, Assigned: billm)

References

Details

Attachments

(5 files, 2 obsolete files)

+++ This bug was initially created as a clone of Bug #1139696 +++

Structured clones of RegExp and Set works with a data URI content script and when the content page is a data URI, it doesn't work for http pages. See the attached testcase which outputs to the Browser Console on pages with <input type=password>. WARNING: saved logins for the site are output to the console. 

Works on: data:text/html,<form><input type=password ></form>
Doesn't work on: https://wiki.allizom.org/index.php?title=Special:UserLogin

Actual result:
> "data" Object { requestId: "{8816d4cc-07ba-9e43-88fb-abb79add38…", logins: Array[0], set: Object, regexp: Object, array: Array[3] }
> "set" Object {  } "[object Object]"
> "regexp" Object {  } "[object Object]"

Expected result:
> "data" Object { requestId: "{cc86a722-acf7-4941-96b5-7868ee217f…", logins: Array[0], set: Set[3], regexp: /four/ }
> "set" Set [ 1, /two/, 3 ] "[object Set]"
> "regexp" /four/ "[object RegExp]"

It seems like some kind of security wrapper is getting in the way.
Flags: needinfo?(bobbyholley)
(In reply to Matthew N. [:MattN] from comment #0)
> Structured clones of RegExp and Set works with a data URI content script and
> when the content page is a data URI, it doesn't work for http pages. See the
> attached testcase which outputs to the Browser Console on pages with <input
> type=password>. WARNING: saved logins for the site are output to the
> console. 

Can you be more explicit about which principal is cloning the object, and which principal the object belongs to?

Can you reproduce the problem using only Sandboxes? That would let you write a self-contained testcase of a few lines, at which point I might be able to take a look.
Flags: needinfo?(bobbyholley)
Hey Matt, I couldn't reproduce this one. I applied your patch and visited the allizom page you gave. It printed the expected output.

The test here only deals with the system principal. Did you maybe reduce it too much by accident?
Flags: needinfo?(MattN+bmo)
Weird, I also can't reproduce now… I'll try figure out what changed.
OK, the problem is that it only fails when you have a saved login for the domain (I assumed it was related to the data URI null principal since all the sites I tested on had saved logins). An array of nsILoginInfo is getting sent in the same message if there are ones saved for that domain.

You can make the existing testcase work by submitting a fake username and password at the URL and choosing to have Firefox remember it. The next page load will have the bug. I will also make a reduced testcase.
Flags: needinfo?(MattN+bmo)
Summary: Structured clone of Set and Regexp doesn't work via sendAsyncMessage depending on the principal of the content page → Structured clone of Set and Regexp doesn't work via sendAsyncMessage if XPCOM objects are also sent
OK, this is a known problem. If structured clone fails (and it will fail on an nsILoginInfo, as it should) then we fall back to sending the message using JSON. JSON doesn't support Set.

We've run into a couple bugs caused by this and I don't know why we do it. The code is here:
http://hg.mozilla.org/mozilla-central/annotate/a27dd304348d/dom/base/nsFrameMessageManager.cpp#l590

It was introduced in bug 759427. Ben, do you know why we do this? It's really a very awful thing.
Flags: needinfo?(bent.mozilla)
You can run this in the Browser Environment of Scratchpad.
Never mind. Ben didn't write that patch, Olli did. https://bugzilla.mozilla.org/show_bug.cgi?id=759427#c40 kind of explains it. nsIDOMContactFindOptions no longer exists. Olli, do you think we could remove this code?
Flags: needinfo?(bent.mozilla) → needinfo?(bugs)
It would be better if it fell back to JSON only on the XPCOM object that isn't cloneable instead of making the whole data argument JSON. If we keep the behaviour at least a warning would be good.

So, XPCOM objects aren't supposed to be sent I guess but this is how mrbkap make password manager work last year… Should we teach Gecko how to clone nsILoginInfo or just convert to an object ourselves before sending?
Hopefully we can get rid of the JSON entirely, but doing each property by itself would be a good fallback.

I think it would be best to convert nsILoginInfo to an object before sending. That's all this code is doing anyway (via JSON.parse(JSON.stringify(loginInfo))).
OK, I was hoping there was a way an XPCOM object could make itself structured cloneable so we wouldn't have to do the conversion on both sides.
See Also: → 1140242
(In reply to Bill McCloskey (:billm) from comment #7)
> Never mind. Ben didn't write that patch, Olli did.
> https://bugzilla.mozilla.org/show_bug.cgi?id=759427#c40 kind of explains it.
> nsIDOMContactFindOptions no longer exists. Olli, do you think we could
> remove this code? 
That wouldn't of course fix this issue where someone is trying to send unsupported nsIFoo in the message.
But sure, Contacts API is webidl-fied these days, and
http://mxr.mozilla.org/mozilla-central/source/dom/contacts/ContactManager.js?rev=aa2ab2b3f01b&mark=358-358,361-363#355
ends up passing just plain normal JS object to sendAsyncMessage, not nsIFoo.
So we could try to remove the JSON fallback, if we aren't sending random nsIFoos. First thing is perhaps to 
start warning about falling back to JSON.


MattN, you might want to ask baku about the options to easily support
random nsIFoo in structured cloning.
Flags: needinfo?(bugs)
I tried to remove the JSON code entirely, but there are too many places where we depend on it. This patch does sorta what Matt suggested and converts individual properties to JSON and back only if they can't be cloned. This should fix most of the problems where people accidentally run into this.
Assignee: nobody → wmccloskey
Status: NEW → ASSIGNED
Attachment #8580892 - Flags: review?(bugs)
Attached patch testSplinter Review
I turned Matt's scratchpad code into a test.
Attachment #8580894 - Flags: review?(bugs)
Attachment #8580894 - Flags: review?(bugs) → review+
Comment on attachment 8580892 [details] [diff] [review]
clone each property independently

This looks like a rather slow path. Can we please warn aggressively here?
People really shouldn't try to send random stuff using mm messages.


I think we should not do this. We should move towards using structured cloning all the time and aim for removing the json hack. This patch is just adding more hacks. What next, recursively do the same hack?

Feel free to explain why you want this approach (and ask review again)
Attachment #8580892 - Flags: review?(bugs) → review-
Well, I have a stack of patches that fix most of the problems. Here's what it looks like:

4a8b002 Fix dom/apps
f58d6ac Fix special powers
a12ff57 Fix content.js
b884afa Fix nsDOMIdentity.js
4689101 Fix content pref service
31fb9ee Fix password mgr
4603355 Fix addon sdk
629596f Fix dom/contacts

However, there are still failures in some Python Gaia test suite that I have no idea how to debug (the same test suite as bug 1131375).

Most of the patches above end up explicitly running JSON.parse(JSON.stringify(...)) on the message since it's the easiest way to solve the problem. So eventually I got fed up and wrote this patch.

I'd be fine printing a warning though. I'll work on that soon.
Attached patch patch v2 (obsolete) — Splinter Review
This patch includes a warning.
Attachment #8580892 - Attachment is obsolete: true
Attachment #8582803 - Flags: review?(bugs)
Comment on attachment 8582803 [details] [diff] [review]
patch v2

Wrong patch? This is the test patch I reviewed already.
Attachment #8582803 - Flags: review?(bugs)
Attached patch patch v3Splinter Review
Sorry about that.
Attachment #8582803 - Attachment is obsolete: true
Attachment #8583885 - Flags: review?(bugs)
I still don't understand why we want to add more hacks.
Comment on attachment 8583885 [details] [diff] [review]
patch v3

So I'd just add a warning, and then eventually remove the json-hack, since
I'm not aware of the new hack to solve any actually issues.
Am I missing something here?

"Sending message that cannot be cloned." could perhaps hint that the
message contains uncloneable properties, possibly random xpcom objects.
Attachment #8583885 - Flags: review?(bugs) → review-
Why don't you use a write-callback that implements the json-hack, instead of manually doing it for every property?
Attached patch patch v4Splinter Review
This presumes people will look at the error console, but maybe they do. I dunno.
Attachment #8585754 - Flags: review?(bugs)
If they don't, I do, and complain :)
(In reply to Bill McCloskey (:billm) from comment #22)
> This presumes people will look at the error console, but maybe they do.

It's the first place front-end developers look when there are problems. It's also where we tell bug reporters to look too.
Attachment #8585754 - Flags: review?(bugs) → review+
https://hg.mozilla.org/mozilla-central/rev/0fe2a4aa6e36
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
You need to log in before you can comment on or make changes to this bug.