Last Comment Bug 780686 - DOMApplicationRegistry._cloneAppObject doesn't clone the `receipts` array
: DOMApplicationRegistry._cloneAppObject doesn't clone the `receipts` array
Product: Core
Classification: Components
Component: DOM: Apps (show other bugs)
: unspecified
: All All
-- normal (vote)
: mozilla17
Assigned To: Myk Melez [:myk] [@mykmelez]
: [:fabrice] Fabrice Desré
Depends on:
  Show dependency treegraph
Reported: 2012-08-06 12:32 PDT by Myk Melez [:myk] [@mykmelez]
Modified: 2012-08-12 08:33 PDT (History)
3 users (show)
ryanvm: in‑testsuite-
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

patch v1: clones `receipts` array (504 bytes, patch)
2012-08-06 12:32 PDT, Myk Melez [:myk] [@mykmelez]
fabrice: review+
myk: checkin+
Details | Diff | Splinter Review

Description User image Myk Melez [:myk] [@mykmelez] 2012-08-06 12:32:55 PDT
Created attachment 649351 [details] [diff] [review]
patch v1: clones `receipts` array

DOMApplicationRegistry._cloneAppObject doesn't clone the `receipts` array, which it obtains from content.

This causes DOMWindows that install apps to leak due to bug 780674 (which should get fixed by the eventual fix for that bug).  It also makes it possible for a webapp to implicitly modify an app's receipts after installing the app, which seems undesirable.  And, more generally, it potentially misleads callers who expect the method to deeply clone the app object, such that the clone contains no references to parts of the original object.

Here's a fix that uses JSON to clone the array.
Comment 1 User image Myk Melez [:myk] [@mykmelez] 2012-08-07 13:57:44 PDT
Comment on attachment 649351 [details] [diff] [review]
patch v1: clones `receipts` array
Comment 2 User image [:fabrice] Fabrice Desré 2012-08-07 15:57:21 PDT
Small followup:
Comment 3 User image Ryan VanderMeulen [:RyanVM] 2012-08-07 17:50:52 PDT
Awesome, so this was backed out along with bug 772299 due to mochitest-other permaorange.

After backing out, I realized that Fabrice's follow-up fixed the orange. Please be starring builds when pushing bustage fixes...

Re-pushed (with the follow-up included).

Note You need to log in before you can comment on or make changes to this bug.