Content-proxy tries to extend non extensible Objects

RESOLVED FIXED

Status

Add-on SDK
General
P1
normal
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: hazeuh@gmail.com, Assigned: ochameau)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Reporter)

Description

7 years ago
Created attachment 545434 [details]
A small add-on that reproduces this bug.

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:5.0.1) Gecko/20100101 Firefox/5.0.1
Build ID: 20110707182747

Steps to reproduce:

I tried to to do Canvas manipulation using amongst others a Uint8ClampedArray object.
That object is not extensible.


Actual results:

My script crashes as content-proxy tries to extend my Uint8ClampedArray.


Expected results:

I'd suggest that maybe content-proxy shouldn't proxify inextensible objects, but I have a too little understanding of content-proxy to be sure that would be intended behaviour.

Regardless, adding
if(!Object.isExtensible(value)) return value;
in content-proxy.js at line 140 seems to fix the bug for me.
(Reporter)

Comment 1

7 years ago
As an additional note, I think this issue breaks all canvas manipulation from add-on sdk.
Alex: can you confirm this bug, and is hazeuh's solution a reasonable way to fix it?
Whiteboard: [triage:followup]
Any idea if this really is a bug, Alex?

I'm tempted to close it out since it's been sitting here unconfirmed for two months...
(Reporter)

Comment 4

7 years ago
As of today, Firefox 6 and Add-on SDK 1.1RC, this bug is still present.

I found a workaround for my needs though, through unsafeWindow, thanks to this blog post http://blog.mozilla.com/addons/2011/09/01/add-on-sdk-faq-content-script-access/
 
But as told in that post, it's not supported API and may be removed in the future, so maybe one day I'll come back to you for a fix. :)

If you wish additional info, please feel free to contact me.
(Assignee)

Comment 5

7 years ago
Oh I missed this bug when myk pinged me...

I don't need to verify this as there is very high probability that proxies broke such API! That been said, I don't think that the fix mentioned in first comment is safe, I'll need more time to verify this. I'll take a look at this next week, as I'm using this work week to work on stuff that need discussion with US folks.
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
(Assignee)

Updated

7 years ago
Assignee: nobody → poirot.alex
Priority: -- → P1
Whiteboard: [triage:followup]
Alex, any update on this?
(Assignee)

Comment 7

7 years ago
Sorry, no update, I still didn't had time to focus on non-features bug, 
but I'm planning to dedicate multiple days to fix multiple bugs like this one!
(Assignee)

Comment 8

7 years ago
Thanks for your report!
You are right, we do not need to proxify typed array: it is useless and slow performances where it is needed!
But we should not prevent proxies for all non-extensible objects as they may return non-primitive values.
(Assignee)

Comment 9

7 years ago
Created attachment 574906 [details] [diff] [review]
Fix typed arrays in content scripts

Fix + related unit test.
Attachment #574906 - Flags: review?(rFobic)
Comment on attachment 574906 [details] [diff] [review]
Fix typed arrays in content scripts

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

Looks good!
Attachment #574906 - Flags: review?(rFobic) → review+
(Assignee)

Updated

7 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.