Closed
Bug 1139718
Opened 6 years ago
Closed 6 years ago
Structured clone of Set and Regexp doesn't work via sendAsyncMessage if XPCOM objects are also sent
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla40
People
(Reporter: MattN, Assigned: billm)
References
Details
Attachments
(5 files, 2 obsolete files)
5.13 KB,
patch
|
Details | Diff | Splinter Review | |
785 bytes,
application/javascript
|
Details | |
2.13 KB,
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
3.94 KB,
patch
|
smaug
:
review-
|
Details | Diff | Splinter Review |
2.10 KB,
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
+++ 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)
Comment 1•6 years ago
|
||
(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)
![]() |
||
Updated•6 years ago
|
Assignee | ||
Comment 2•6 years ago
|
||
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)
Reporter | ||
Comment 3•6 years ago
|
||
Weird, I also can't reproduce now… I'll try figure out what changed.
Reporter | ||
Comment 4•6 years ago
|
||
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
Assignee | ||
Comment 5•6 years ago
|
||
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)
Reporter | ||
Comment 6•6 years ago
|
||
You can run this in the Browser Environment of Scratchpad.
Assignee | ||
Comment 7•6 years ago
|
||
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)
Reporter | ||
Comment 8•6 years ago
|
||
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?
Assignee | ||
Comment 9•6 years ago
|
||
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))).
Reporter | ||
Comment 10•6 years ago
|
||
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•6 years ago
|
||
(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)
Assignee | ||
Comment 12•6 years ago
|
||
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 | ||
Comment 13•6 years ago
|
||
I turned Matt's scratchpad code into a test.
Attachment #8580894 -
Flags: review?(bugs)
Updated•6 years ago
|
Attachment #8580894 -
Flags: review?(bugs) → review+
Comment 14•6 years ago
|
||
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-
Assignee | ||
Comment 15•6 years ago
|
||
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.
Assignee | ||
Comment 16•6 years ago
|
||
This patch includes a warning.
Attachment #8580892 -
Attachment is obsolete: true
Attachment #8582803 -
Flags: review?(bugs)
Comment 17•6 years ago
|
||
Comment on attachment 8582803 [details] [diff] [review] patch v2 Wrong patch? This is the test patch I reviewed already.
Attachment #8582803 -
Flags: review?(bugs)
Assignee | ||
Comment 18•6 years ago
|
||
Sorry about that.
Attachment #8582803 -
Attachment is obsolete: true
Attachment #8583885 -
Flags: review?(bugs)
Comment 19•6 years ago
|
||
I still don't understand why we want to add more hacks.
Comment 20•6 years ago
|
||
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-
Comment 21•6 years ago
|
||
Why don't you use a write-callback that implements the json-hack, instead of manually doing it for every property?
Assignee | ||
Comment 22•6 years ago
|
||
This presumes people will look at the error console, but maybe they do. I dunno.
Attachment #8585754 -
Flags: review?(bugs)
Comment 23•6 years ago
|
||
If they don't, I do, and complain :)
Reporter | ||
Comment 24•6 years ago
|
||
(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.
Updated•6 years ago
|
Attachment #8585754 -
Flags: review?(bugs) → review+
Assignee | ||
Comment 25•6 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/0fe2a4aa6e36
Comment 26•6 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/0fe2a4aa6e36
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox40:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
You need to log in
before you can comment on or make changes to this bug.
Description
•