Bug in dojo/dom-style

[Jan Misker]

Hi all,
I noticed a bug in dojo/dom-style in combination with Firefox (51.0.1).
When I call getComputedStyle execution halts, and I get a warning
  Permission denied to access property "getComputedStyle"
Note that this happens only cross-domain, so check this test-case (difficult to create one on JSFiddle):
http://dev.prettigparkeren.nl/test-embed/test.html
 
The issues seems to be resolved by not referencing window.parent but just the window (which makes sense from a same-origin perspective). I described it here as well
https://github.com/dojo/dojo/commit/bfea978ffd6e7a0606ad677e457e7079e42a937d#commitcomment-20895564

But I don't know whether the code references .parent for a reason?

best

Jan Misker

--
Dojo Toolkit: http://dojotoolkit.org/
Tutorials: http://dojotoolkit.org/documentation/

[hidden email]
To unsubscribe, visit: http://mail.dojotoolkit.org/mailman/listinfo/dojo-interest

[dylanks]

With two similar reports in the past 24 hours (from Jan Misker and Nick
Fenwick), I've opened https://bugs.dojotoolkit.org/ticket/18973

If anyone has time for a PR, I'll review it quickly, otherwise I'll aim
to look into it this weekend.

Regards,
-Dylan

on 2/15/17, 08:19 (GMT-07:00) Jan Misker said the following:

> Hi all,
> I noticed a bug in dojo/dom-style in combination with Firefox (51.0.1).
> When I call getComputedStyle execution halts, and I get a warning
>   Permission denied to access property "getComputedStyle"
> Note that this happens only cross-domain, so check this test-case (difficult to create one on JSFiddle):
> http://dev.prettigparkeren.nl/test-embed/test.html
>  
> The issues seems to be resolved by not referencing window.parent but just the window (which makes sense from a same-origin perspective). I described it here as well
> https://github.com/dojo/dojo/commit/bfea978ffd6e7a0606ad677e457e7079e42a937d#commitcomment-20895564
>
> But I don't know whether the code references .parent for a reason?
>
> best
>
> Jan Misker
>

--
Dojo Toolkit: http://dojotoolkit.org/
Tutorials: http://dojotoolkit.org/documentation/

[hidden email]
To unsubscribe, visit: http://mail.dojotoolkit.org/mailman/listinfo/dojo-interest

[neekfenwick]

Really appreciate it Dylan.  I'm extremely busy at the moment.  I have
managed to work around the problem with this patch (loaded very early in
the app startup, which wraps dom-style/getComputedStyle to prevent it
from crashing on IE):

require(['dojo/dom-style', 'dojo/aspect', 'dojo/sniff'], function
(domStyle, aspect, has) {

     console.log('tfs15017-dom-style patch here! ie? ', has('ie'));
     // has('ie') seems unreliable in ie11 :(
     // if (!has('ie')) {
     //     // Patch only required for ie
     //     return;
     // }

     aspect.around(domStyle, 'getComputedStyle', function(originalMethod){
         return function(node){
             var res;
           try {
               res = originalMethod(node);
           } catch (e) {
               console.error('getComputedStyle wrapper caught exception:
', e);
               res = window.getComputedStyle(node, null);
           }
           return res;
         };
     });
     console.log('tfs15017-dom-style patch applied!');
});

It's not a real fix, it's a horrible workaround, but it does seem to
mean the code will execute properly on IE.  Note the commented out
attempt to use has('ie'), doesn't work, on my IE 11 the test in
dojo/sniff for "if(document.all && !has("opera")){" returns false and I
got sick of it at that point and just applied my patch for all browsers,
most of which won't throw in the originalMethod() invocation of
getComputedStyle.

I'll deploy this to production tomorrow morning to placate IE browsers
around the world, and may God forgive me :P

Nick

On 15/02/17 23:36, Dylan Schiemann wrote:

> With two similar reports in the past 24 hours (from Jan Misker and Nick
> Fenwick), I've opened https://bugs.dojotoolkit.org/ticket/18973
>
> If anyone has time for a PR, I'll review it quickly, otherwise I'll aim
> to look into it this weekend.
>
> Regards,
> -Dylan
>
> on 2/15/17, 08:19 (GMT-07:00) Jan Misker said the following:
>> Hi all,
>> I noticed a bug in dojo/dom-style in combination with Firefox (51.0.1).
>> When I call getComputedStyle execution halts, and I get a warning
>>    Permission denied to access property "getComputedStyle"
>> Note that this happens only cross-domain, so check this test-case (difficult to create one on JSFiddle):
>> http://dev.prettigparkeren.nl/test-embed/test.html
>>  
>> The issues seems to be resolved by not referencing window.parent but just the window (which makes sense from a same-origin perspective). I described it here as well
>> https://github.com/dojo/dojo/commit/bfea978ffd6e7a0606ad677e457e7079e42a937d#commitcomment-20895564
>>
>> But I don't know whether the code references .parent for a reason?
>>
>> best
>>
>> Jan Misker
>>

--
Dojo Toolkit: http://dojotoolkit.org/
Tutorials: http://dojotoolkit.org/documentation/

[hidden email]
To unsubscribe, visit: http://mail.dojotoolkit.org/mailman/listinfo/dojo-interest

[Jan Misker]

I've had reports from users about bugs in IE as well, so indeed this is likely the same issue. I wasn't subscribed to the list so I missed the report by Nick.

I tested with dojo 1.11.2 as well (last version in googleapis cdn) and then it works, so it seems this issue was introduced with the commit I referenced earlier.

See my comment on that commit for a very simple but untested/uninformed fix that works for me in production.
>>> https://github.com/dojo/dojo/commit/bfea978ffd6e7a0606ad677e457e7079e42a937d#commitcomment-20895564

best
Jan

> On 15 Feb 2017, at 17:53, Nick Fenwick <[hidden email]> wrote:
>
> Really appreciate it Dylan.  I'm extremely busy at the moment.  I have
> managed to work around the problem with this patch (loaded very early in
> the app startup, which wraps dom-style/getComputedStyle to prevent it
> from crashing on IE):
>
> require(['dojo/dom-style', 'dojo/aspect', 'dojo/sniff'], function
> (domStyle, aspect, has) {
>
>     console.log('tfs15017-dom-style patch here! ie? ', has('ie'));
>     // has('ie') seems unreliable in ie11 :(
>     // if (!has('ie')) {
>     //     // Patch only required for ie
>     //     return;
>     // }
>
>     aspect.around(domStyle, 'getComputedStyle', function(originalMethod){
>         return function(node){
>             var res;
>           try {
>               res = originalMethod(node);
>           } catch (e) {
>               console.error('getComputedStyle wrapper caught exception:
> ', e);
>               res = window.getComputedStyle(node, null);
>           }
>           return res;
>         };
>     });
>     console.log('tfs15017-dom-style patch applied!');
> });
>
> It's not a real fix, it's a horrible workaround, but it does seem to
> mean the code will execute properly on IE.  Note the commented out
> attempt to use has('ie'), doesn't work, on my IE 11 the test in
> dojo/sniff for "if(document.all && !has("opera")){" returns false and I
> got sick of it at that point and just applied my patch for all browsers,
> most of which won't throw in the originalMethod() invocation of
> getComputedStyle.
>
> I'll deploy this to production tomorrow morning to placate IE browsers
> around the world, and may God forgive me :P
>
> Nick
>
> On 15/02/17 23:36, Dylan Schiemann wrote:
>> With two similar reports in the past 24 hours (from Jan Misker and Nick
>> Fenwick), I've opened https://bugs.dojotoolkit.org/ticket/18973
>>
>> If anyone has time for a PR, I'll review it quickly, otherwise I'll aim
>> to look into it this weekend.
>>
>> Regards,
>> -Dylan
>>
>> on 2/15/17, 08:19 (GMT-07:00) Jan Misker said the following:
>>> Hi all,
>>> I noticed a bug in dojo/dom-style in combination with Firefox (51.0.1).
>>> When I call getComputedStyle execution halts, and I get a warning
>>>   Permission denied to access property "getComputedStyle"
>>> Note that this happens only cross-domain, so check this test-case (difficult to create one on JSFiddle):
>>> http://dev.prettigparkeren.nl/test-embed/test.html
>>>
>>> The issues seems to be resolved by not referencing window.parent but just the window (which makes sense from a same-origin perspective). I described it here as well
>>> https://github.com/dojo/dojo/commit/bfea978ffd6e7a0606ad677e457e7079e42a937d#commitcomment-20895564
>>>
>>> But I don't know whether the code references .parent for a reason?
>>>
>>> best
>>>
>>> Jan Misker
>>>
>
> --
> Dojo Toolkit: http://dojotoolkit.org/
> Tutorials: http://dojotoolkit.org/documentation/
>
> [hidden email]
> To unsubscribe, visit: http://mail.dojotoolkit.org/mailman/listinfo/dojo-interest

--
Dojo Toolkit: http://dojotoolkit.org/
Tutorials: http://dojotoolkit.org/documentation/

[hidden email]
To unsubscribe, visit: http://mail.dojotoolkit.org/mailman/listinfo/dojo-interest

[dylanks]

To answer your question about has IE, IE11 asked to not be called IE, so
there's a has('trident') check that will match true for IE11. See
http://dojo-toolkit.33424.n3.nabble.com/Dojo-1-9-2-now-available-td4000355.html
for more on that fun!

on 2/15/17, 09:53 (GMT-07:00) Nick Fenwick said the following:

> Really appreciate it Dylan.  I'm extremely busy at the moment.  I have
> managed to work around the problem with this patch (loaded very early in
> the app startup, which wraps dom-style/getComputedStyle to prevent it
> from crashing on IE):
>
> require(['dojo/dom-style', 'dojo/aspect', 'dojo/sniff'], function
> (domStyle, aspect, has) {
>
>      console.log('tfs15017-dom-style patch here! ie? ', has('ie'));
>      // has('ie') seems unreliable in ie11 :(
>      // if (!has('ie')) {
>      //     // Patch only required for ie
>      //     return;
>      // }
>
>      aspect.around(domStyle, 'getComputedStyle', function(originalMethod){
>          return function(node){
>              var res;
>            try {
>                res = originalMethod(node);
>            } catch (e) {
>                console.error('getComputedStyle wrapper caught exception:
> ', e);
>                res = window.getComputedStyle(node, null);
>            }
>            return res;
>          };
>      });
>      console.log('tfs15017-dom-style patch applied!');
> });
>
> It's not a real fix, it's a horrible workaround, but it does seem to
> mean the code will execute properly on IE.  Note the commented out
> attempt to use has('ie'), doesn't work, on my IE 11 the test in
> dojo/sniff for "if(document.all && !has("opera")){" returns false and I
> got sick of it at that point and just applied my patch for all browsers,
> most of which won't throw in the originalMethod() invocation of
> getComputedStyle.
>
> I'll deploy this to production tomorrow morning to placate IE browsers
> around the world, and may God forgive me :P
>
> Nick
>
> On 15/02/17 23:36, Dylan Schiemann wrote:
>> With two similar reports in the past 24 hours (from Jan Misker and Nick
>> Fenwick), I've opened https://bugs.dojotoolkit.org/ticket/18973
>>
>> If anyone has time for a PR, I'll review it quickly, otherwise I'll aim
>> to look into it this weekend.
>>
>> Regards,
>> -Dylan
>>
>> on 2/15/17, 08:19 (GMT-07:00) Jan Misker said the following:
>>> Hi all,
>>> I noticed a bug in dojo/dom-style in combination with Firefox (51.0.1).
>>> When I call getComputedStyle execution halts, and I get a warning
>>>    Permission denied to access property "getComputedStyle"
>>> Note that this happens only cross-domain, so check this test-case (difficult to create one on JSFiddle):
>>> http://dev.prettigparkeren.nl/test-embed/test.html
>>>  
>>> The issues seems to be resolved by not referencing window.parent but just the window (which makes sense from a same-origin perspective). I described it here as well
>>> https://github.com/dojo/dojo/commit/bfea978ffd6e7a0606ad677e457e7079e42a937d#commitcomment-20895564
>>>
>>> But I don't know whether the code references .parent for a reason?
>>>
>>> best
>>>
>>> Jan Misker
>>>
>

--
Dojo Toolkit: http://dojotoolkit.org/
Tutorials: http://dojotoolkit.org/documentation/

[hidden email]
To unsubscribe, visit: http://mail.dojotoolkit.org/mailman/listinfo/dojo-interest