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

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Matthew N. [:MattN]]

Attachment: Testcase patch that outputs to the Browser Console on pages with <input type=password>

+++ 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.

Comment 1

[Bobby Holley (:bholley)]

(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.

Comment 2

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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?

Comment 3

[Matthew N. [:MattN]]

Weird, I also can't reproduce now… I'll try figure out what changed.

Comment 4

[Matthew N. [:MattN]]

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.

Comment 5

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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.

Comment 6

[Matthew N. [:MattN]]

Attachment: Browser Scratchpad Testcase

You can run this in the Browser Environment of Scratchpad.

Comment 7

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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?

Comment 8

[Matthew N. [:MattN]]

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?

Comment 9

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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))).

Comment 10

[Matthew N. [:MattN]]

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.

Comment 11

[Olli Pettay [:smaug]]

(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.

Comment 12

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: clone each property independently

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.

Comment 13

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: test

I turned Matt's scratchpad code into a test.

Comment 14

[Olli Pettay [:smaug]]

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)

Comment 15

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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.

Comment 16

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: patch v2

This patch includes a warning.

Comment 17

[Olli Pettay [:smaug]]

Comment on attachment 8582803 [details] [diff] [review]
patch v2

Wrong patch? This is the test patch I reviewed already.

Comment 18

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: patch v3

Sorry about that.

Comment 19

[Olli Pettay [:smaug]]

I still don't understand why we want to add more hacks.

Comment 20

[Olli Pettay [:smaug]]

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.

Comment 21

[Tom Schuster [:evilpie]]

Why don't you use a write-callback that implements the json-hack, instead of manually doing it for every property?

Comment 22

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: patch v4

This presumes people will look at the error console, but maybe they do. I dunno.

Comment 23

[Olli Pettay [:smaug]]

If they don't, I do, and complain :)

Comment 24

[Matthew N. [:MattN]]

(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.

Comment 25

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0fe2a4aa6e36

Comment 26

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0fe2a4aa6e36