1184439 - "Deep Freeze" the manifest object in DOMApplication instances

Status: RESOLVED WONTFIX

(Core Graveyard :: DOM: Apps, defect)

Description

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Please see bug 1176184 comment 12 and related discussion titled "Should the entire JS structure be Object.freeze()'d if it's exposed through a read only property?" in dev-platform.

Comment 1

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Attachment: bug1184439.patch

Hi :bholley,

Could you help me on this? The goal of this patch is to prevent content script from modify the JS structure in-place exposed from DOMApplication#manifest, since App API will remove it at a time unknown to content.

When I apply this patch, I will be hit by this error.

https://dxr.mozilla.org/mozilla-central/source/js/xpconnect/wrappers/XrayWrapper.cpp#1820

This is implemented in bug 789897. It looks like it says DOM API properties ("reflectors"? Not sure about the term) should always be extendable by content script? This is entirely opposite to what I want to achieve here.

Could you tell me if I understand this comment correctly and maybe suggest alternative approach to achieve my goal? Thanks!

Comment 2

[Bobby Holley (:bholley)]

If you're hitting this code, it means that the object you're trying to freeze (perhaps recursively) is an XrayWrapper (see [1]).

Presumably you want to freeze the underlying object, right? You could do Object.freeze(cloned.wrappedJSObject), but it seems like something we should support in cloneInto. I'll write a patch.

[1] https://developer.mozilla.org/en-US/docs/Xray_vision

Comment 3

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Attachment: Patch v1

Comment 4

[Tim Guan-tin Chien [:timdream] (please needinfo)]

I've manually verified bug 1186213 did what's desirable here. Thanks Bobby!

https://treeherder.mozilla.org/#/jobs?repo=try&revision=cfe7c949d26b

Comment 5

[[:fabrice] Fabrice Desré]

Comment on attachment 8640934 [details] [diff] [review]
Patch v1

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

::: dom/apps/AppsServiceChild.jsm
@@ +44,5 @@
>      }
>  
>      let winObjs = this._cache[aManifestURL];
>      if (!(aInnerWindowID in winObjs)) {
> +      // Clone a deep frozen version of the manifest so contant couldn't modify

nit: s/contant/content

Comment 6

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Attachment: Patch for commit

Need to wait for Gij to turn green before check-in.

I don't think there is anywhere in Gaia that could have modified the manifest in-place, but I might be wrong and it's good if we could caught it before check-in.

Comment 7

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Tim Guan-tin Chien [:timdream] (slow response; please ni? to queue) from comment #6)
> Created attachment 8641465 [details] [diff] [review]
> Patch for commit
> 
> Need to wait for Gij to turn green before check-in.
> 
> I don't think there is anywhere in Gaia that could have modified the
> manifest in-place, but I might be wrong and it's good if we could caught it
> before check-in.

... my local phone build indeed break the phone. It can't even reach home screen app.

Will look further tomorrow...

Comment 8

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Tim Guan-tin Chien [:timdream] (slow response; please ni? to queue) from comment #7)
> ... my local phone build indeed break the phone. It can't even reach home
> screen app.
> 
> Will look further tomorrow...

No. Wrong patch.

Comment 9

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Looks like in-place editing of manifest object is not something myself can fix. I've sent an email to dev-gaia so everyone can agree which way we should move forward.

Comment 10

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Let's go back to this bug if and only if there is enough traction to fix this.

Comment 11

[Sylvestre Ledru [:Sylvestre]]

Core Graveyard / DOM: Apps is inactive. Closing all bugs in this component.