toString() broken on cross-origin objects

RESOLVED FIXED in Firefox 51

Status

()

defect
RESOLVED FIXED
3 years ago
3 months ago

People

(Reporter: jdavis, Assigned: bzbarsky)

Tracking

({regression, site-compat})

51 Branch
mozilla53
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify ?

Firefox Tracking Flags

(firefox51+ fixed, firefox52 fixed, firefox53 fixed)

Details

(Whiteboard: [webcompat])

Attachments

(3 attachments, 3 obsolete attachments)

Reporter

Description

3 years ago
Mozilla uses Salesforce Marketing Cloud to manage our email programs. It is one of the leading email marketing platforms worldwide - and it's currently broken in Beta, Nightly & Dev.

In filing a bug with the Salesforce team, I got this reply:


Mozilla plans to implement ES6 Symbol.toStringTag property in Firefox version 51. (https://bugzilla.mozilla.org/show_bug.cgi?id=1114580) 

In ES6, Object.prototype.toString does not use as Class meta property anymore, so they are adding Symbol.toStringTag to all DOM prototype objects. This will cause issues in our script where we access an object that is not in the current domain. The error message is "Permission denied to access property Symbol.toStringTag".

If Mozilla release the version as it currently is, there is a potential to see multiple issues with Firefox users. There are multiple apps that would not work.
Looking at Mozilla bug page, it appears that they are aware of this issue as it is a child bug of the Symbol.toStringTag implementation, but according to the site, no work is currently being done. https://bugzilla.mozilla.org/showdependencytree.cgi?id=1114580&hide_resolved=1

Applications that could have issue and possibly not work in Firefox but not limited to are as follow:
Email Studio
MobileConnect 2.0
Web and Mobile Analytics
Approvals Service
Content Builder

According the release schedule, this version is scheduled to go out on January 24, 2017.


Is there a way to ensure that this fix happens before Jan 24th's release? 


Risk: we may loose Firefox users who use Salesforce (more than just Mozilla)
Reporter

Comment 1

3 years ago
Posted image JS_error.png
Thanks for filing, Jessilyn. Moving this into Tech Evangelism so we have a chance to have a look at it and do cross-checks with other browsers before figuring out actions to take.
Component: JavaScript: Standard Library → Desktop
Product: Core → Tech Evangelism
Version: 51 Branch → Firefox 51
Tom, any thoughts about this?
Blocks: 1114580
Flags: needinfo?(evilpies)
Dennis can add STR (I believe he's looking into this), but I want to make sure we track this ASAP, however late in the release cycle. :/
Posted file Simple testcase
I've attached a simple test case and it's hosted on http://stuff.schub.io/mozilla/testcases/1321299/. Click the button to see an exception in Firefox, but "[object Object]" in Chrome.
This is a regression from bug 1114580, where we implemented Symbol.toStringTag. Renaming the title to be more easily understandable and more accurate.
Summary: Salesforce Marketing Cloud breaking because Object.prototype.toString → toString() broken on cross-origin objects
Whiteboard: [webcompat]
Much simpler testcase:

  data:text/html,<iframe src="http://example.com"></iframe><script>onload = function() { try { alert({}.toString.call(frames[0])) } catch (e) { alert(e); } }</script>

This should work per spec.  See https://html.spec.whatwg.org/multipage/browsers.html#crossorigingetownpropertyhelper-(-o,-p-) step 1.
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Comment on attachment 8816294 [details] [diff] [review]
Cross-origin objects should allow gets of certain symbol-named properties but force the value to be undefined

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

So, is this basically what I proposed in bug 1114580#c24? Did that just get dropped on the floor or something?

::: js/xpconnect/wrappers/AccessCheck.cpp
@@ +155,5 @@
>      return CrossOriginOpaque;
>  }
>  
>  bool
> +IdIsCrossOriginFilteredSymbol(JSContext* cx, JS::HandleId id)

Let's call this IsCrossOriginWhitelistedSymbol, and add a comment that the symbol value itself is nerfed.

@@ +195,5 @@
> +               IdIsCrossOriginFilteredSymbol(cx, id)) {
> +        // We always allow access to @@toStringTag, @@hasInstance, and
> +        // @@isConcatSpreadable.  But then we nerf them to be a value descriptor
> +        // with value undefined; this happens in FilteringWrapper's
> +        // FilterPropertyDescriptor.

This comment will need to change.

::: js/xpconnect/wrappers/FilteringWrapper.cpp
@@ +61,5 @@
> +        // of whether our target has them.
> +        //
> +        // XXXbz Should we do this for the cases when we have explicit
> +        // exposedProps exposing these symbols?  It's not clear how to do that,
> +        // exactly...

As much as I really don't care about ExposedProps, this comment does point to the fact that this check doesn't really belong here, and should move to CrossOriginXrayWrapper::getPropertyDescriptor.

Moreover, the whole bit about our target having them or not having them seems to miss the point - this is an XrayWrapper with no waiver or expando object, so what we see from the underlying wrapper is entirely within our own control. And it's not clear to me what code is responsible for making these symbol-valued properties show up. For a normal DOM Xray we'd just inherit it off the prototype, but we don't have a prototype here.

So we need to explicitly add the property in getPropertyDescriptor, but then we also need to add it in getOwnPropertyKeys or whatever it's called these days. I'd think that the underlying wrapper implementation shouldn't supply these properties, so we should be able to strongly assert that they don't exist before stubbing them in.

I'm pretty sure a test for this will fail, and the only reason the tests pass right now is that there is no call to getOwnPropertySymbols. Can you add that please? And maybe a test using Object.getOwnPropertyDescriptors, which IIUC handles both?

::: testing/web-platform/tests/html/browsers/origin/cross-origin-objects/cross-origin-objects.html
@@ +71,5 @@
>   */
>  
> +var whitelistedWindowPropNames = ['location', 'postMessage', 'window', 'frames', 'self', 'top', 'parent',
> +                                  'opener', 'closed', 'close', 'blur', 'focus',
> +                                  'length'];

Nit: Is there a reason to wrap this last item?

@@ +180,5 @@
>    assert_equals(desc.enumerable, false, "property descriptor for " + propName + " should be non-enumerable");
>    assert_equals(desc.configurable, true, "property descriptor for " + propName + " should be configurable");
> +  if (isSymbol) {
> +    assert_true("value" in desc,
> +                "property descriptor for " + propName + " should be a value descriptor");

Shouldn't we also check that it's undefined here?

@@ +270,5 @@
> +addTest(function() {
> +  assert_array_equals(Object.getOwnPropertySymbols(C), [],
> +                      "Object.getOwnPropertySymbols() should not find any symbol-named properties for cross-origin Window");
> +  assert_array_equals(Object.getOwnPropertySymbols(C.location), [],
> +                      "Object.getOwnPropertySymbols() should not find any symbol-named properties for cross-origin Window");

Shouldn't this say "Location"?

::: testing/web-platform/tests/html/browsers/origin/cross-origin-objects/win-documentdomain.sub.html
@@ +1,4 @@
>  <!DOCTYPE html>
>  <html>
>  <head>
> +  <script src="/common/get-host-info.sub.js"></script>

Hm, why is this needed?
Attachment #8816294 - Flags: review?(bobbyholley) → review-
Oh, and of course you _did_ add a test for getOwnPropertySymbols, and that's what the whole spec issue was about. Sorry about that, I was looking at the current version of the file rather than your patch.

I think the spec is basically an oversight, so we should just change it right now and write this patch (and the test) properly, rather than having to come back and do it later.
Track 51+ as this impacts real users who use Salesforce and other applications.
Attachment #8816294 - Attachment is obsolete: true
> So, is this basically what I proposed in bug 1114580#c24?

Pretty similar, yes, independently derived.  ;) 

> Did that just get dropped on the floor or something?

Apparently, yes.  :(

> Let's call this IsCrossOriginWhitelistedSymbol, and add a comment that the symbol value itself is nerfed.

Done.  Comment says:

  // Check whether the given jsid is a symbol whose value can be gotten
  // cross-origin.  Cross-origin gets always return undefined as the value.

> As much as I really don't care about ExposedProps, this comment does
> point to the fact that this check doesn't really belong here, and should
> move to CrossOriginXrayWrapper::getPropertyDescriptor.

Done.

> Nit: Is there a reason to wrap this last item?

Well, my editor did that because 80 chars. ;)  Undone.

> Shouldn't we also check that it's undefined here?

Er... yes!  I meant to, and thought I had, but clearly not.  Good catch.  Fixed.

> Shouldn't this say "Location"?

Yes.  Fixed.

> Hm, why is this needed?

It's not, strictly speaking.   I just ran all the tests in the cross-origin-objects dir (to see what test coverage we needed), found that one of them was marked as timing out _without_ this patch, and fixed it to not do that: it's using a get_port function and without the script that defines that function it ends up with exceptions and times out.

> so we should just change it right now and write this patch (and the test) properly

OK.  Done.
Note that if the final spec ends up defining the ordering expected out of [[OwnPropertyKeys]], then we may need to adjust our code accordingly and can just change the spec to using array_equals.
Component: Desktop → DOM
Product: Tech Evangelism → Core
Version: Firefox 51 → 51 Branch
I meant change the _test_ to using _assert_array_equals_.
Comment on attachment 8816374 [details] [diff] [review]
Cross-origin objects should allow gets of certain symbol-named properties but force the value to be undefined

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

r=me with those fixes.

::: js/xpconnect/wrappers/FilteringWrapper.cpp
@@ +248,5 @@
> +        return false;
> +    }
> +
> +    props.infallibleAppend(
> +      SYMBOL_TO_JSID(JS::GetWellKnownSymbol(cx, JS::SymbolCode::toStringTag)));

Worried about duplicating this list with the one in IsCrossOriginWhitelistedSymbol. Can you make a static array of the SymbolCodes and then iterate over that in both places?

::: testing/web-platform/tests/html/browsers/origin/cross-origin-objects/cross-origin-objects.html
@@ +267,5 @@
>                        "Object.getOwnPropertyNames() gives the right answer for cross-origin Location");
>  }, "[[OwnPropertyKeys]] should return all properties from cross-origin objects");
>  
> +// Compare two arrays that need to have the same elements but may be in
> +// different orders.

Nit: I think it would be a bit more idiomatic to define a symbol comparator, and sort both the reference and test arrays.
Attachment #8816374 - Flags: review?(bobbyholley) → review+

Comment 18

3 years ago
Pushed by bzbarsky@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5f494ff3b83b
Cross-origin objects should allow gets of certain symbol-named properties but force the value to be undefined.  r=bholley
> Can you make a static array of the SymbolCodes and then iterate over that in both places?

Done.

> I think it would be a bit more idiomatic to define a symbol comparator

Yes, but I haven't found any way to define one that makes any sense, unless I assume the symbols are all built-in ones.  I'll add a comment.
Attachment #8816375 - Attachment is obsolete: true
Attachment #8816374 - Attachment is obsolete: true
Comment on attachment 8816550 [details] [diff] [review]
Cross-origin objects should allow gets of certain symbol-named properties but force the value to be undefined

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1114580
[User impact if declined]: Some sites won't work.
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: Not yet.
[Needs manual test from QE? If yes, steps to reproduce]: Verifying that this
   fixes the SalesForce site would be good.  I don't have steps to reproduce,
   but maybe Jessilyn does.
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: Slightly.
[Why is the change risky/not risky?]: Changes web-visible behavior.  That said,
   the new behavior is one that other browsers (e.g. Chrome) already ship and
   is the one we had decided on a while ago to avoid breaking sites.  The real
   question is whether this is more or less risky than backing out bug 1114580.
   I suspect it's less risky.
[String changes made/needed]: None.
Flags: needinfo?(jdavis)
Attachment #8816550 - Flags: approval-mozilla-beta?
Attachment #8816550 - Flags: approval-mozilla-aurora?
Reporter

Comment 22

3 years ago
Dennis Schubert has login access to Salesforce to help troubleshoot this bug. Please let him know what version of Firefox to use and he can verify if the fix works.  Thanks!
Flags: needinfo?(jdavis) → needinfo?(dschubert)
I guess tomorrow's nightly might have the changes, depending on when sheriffs merge stuff.

If Dennis wants to test a tinderbox build and tells me what OS I can link him to a relevant binary.
I have confirmed the patch using tinderbox build 1480710284. SalesForce is working, and so are all the testcases.

Everyone, thank you for reacting this fast!
Flags: needinfo?(dschubert)
Reporter

Comment 25

3 years ago
\o/ Yes! Thank you all!!

Comment 26

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/5f494ff3b83b
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Comment on attachment 8816550 [details] [diff] [review]
Cross-origin objects should allow gets of certain symbol-named properties but force the value to be undefined

web compat fix, take in aurora52
Attachment #8816550 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Flags: needinfo?(evilpies)
bz, it looks like beta is also affected too (bug 1114580 says it was fixed in 51 and 51 is marked affected here).  Do you think we can get this in before we release next week?
Flags: needinfo?(bzbarsky)
Yes, I know it's affected; that's why I asked for beta approval 6 days ago.  As soon as someone gets around to approving this for beta, I'll land it...
Flags: needinfo?(bzbarsky)
(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #30)
> Yes, I know it's affected; that's why I asked for beta approval 6 days ago. 
> As soon as someone gets around to approving this for beta, I'll land it...

Sorry!  I missed the beta? flag somehow...
Comment on attachment 8816550 [details] [diff] [review]
Cross-origin objects should allow gets of certain symbol-named properties but force the value to be undefined

Fix a web compatibility issue. Beta51+. Should be in 51 beta 7.
Attachment #8816550 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #21)
> [Needs manual test from QE? If yes, steps to reproduce]: Verifying that this
>    fixes the SalesForce site would be good.  I don't have steps to reproduce,
>    but maybe Jessilyn does.

Jessilyn, is there anything manual QA can do here to help?
Flags: qe-verify?
Flags: needinfo?(jdavis)
As Jessilyn wrote in comment 22, you'd need to have access to a Salesforce system in order to see the original issue. There is, however, a testcase in comment 7. If you run it before the fix, you'll see an exception. After the fix, it'll show "[object Object]".

I am not aware of any other "real world issues" with this so far, so I don't think there's an easy way to verify the fix besides the tests mentioned above.
Flags: needinfo?(jdavis)
Component: DOM → DOM: Core & HTML
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.