is "window.parent && window.parent.postMessage" check buggy?

[zepumph]

With @samreid recently while running CAF in phet-io mode, we noticed that ColorProfile.reportColor() was sending a postMessage to itself. That was being picked up by the phet-io message listener, producing some buggy data stream.

PhET-iO Events Stream

31 chargesAndFields.controller.input.mouseUpEmitter emitted {"args":[{"x":270.3999938964844,"y":407},{"pointerId":1,"pointerType":"mouse","clientX":270.3999938964844,"clientY":407,"button":0,"which":1}]} 32 chargesAndFields.globals.options.projectorCheckbox.toggledEmitter emitted {"args":[true]} 33 chargesAndFields.globals.projectorColorsProperty changed {"oldValue":false,"newValue":true} 34 chargesAndFields.colorProfile.profileNameProperty changed {"oldValue":"default","newValue":"projector"} 35 chargesAndFields.phetioCommandProcessor invoked {"type":"reportColor","name":"background","value":"#ffffff","phetioEventType":"wrapper"} 36 chargesAndFields.phetioCommandProcessor invoked {"type":"reportColor","name":"checkbox","value":"#000000","phetioEventType":"wrapper"} 37 chargesAndFields.phetioCommandProcessor invoked {"type":"reportColor","name":"checkboxBackground","value":"#ffffff","phetioEventType":"wrapper"} 38 chargesAndFields.phetioCommandProcessor invoked {"type":"reportColor","name":"controlPanelBorder","value":"#c0c0c0","phetioEventType":"wrapper"} 39 chargesAndFields.phetioCommandProcessor invoked {"type":"reportColor","name":"controlPanelFill","value":"#eeeeee","phetioEventType":"wrapper"} 40 chargesAndFields.phetioCommandProcessor invoked {"type":"reportColor","name":"controlPanelText","value":"#000000","phetioEventType":"wrapper"} 41 chargesAndFields.phetioCommandProcessor invoked {"type":"reportColor","name":"electricFieldGridSaturation","value":"#ff0000","phetioEventType":"wrapper"} 42 chargesAndFields.phetioCommandProcessor invoked {"type":"reportColor","name":"electricFieldGridZero","value":"#ffffff","phetioEventType":"wrapper"} 43 chargesAndFields.phetioCommandProcessor invoked {"type":"reportColor","name":"electricFieldSensorCircleFill","value":"#ff9900","phetioEventType":"wrapper"} 44 chargesAndFields.phetioCommandProcessor invoked {"type":"reportColor","name":"electricFieldSensorCircleStroke","value":"#000000","phetioEventType":"wrapper"} 45 chargesAndFields.phetioCommandProcessor invoked {"type":"reportColor","name":"electricFieldSensorLabel","value":"#000000","phetioEventType":"wrapper"} 46 chargesAndFields.phetioCommandProcessor invoked {"type":"reportColor","name":"electricPotentialGridSaturationPositive","value":"#d20000","phetioEventType":"wrapper"} 47 chargesAndFields.phetioCommandProcessor invoked {"type":"reportColor","name":"electricPotentialGridZero","value":"#ffffff","phetioEventType":"wrapper"} 48 chargesAndFields.phetioCommandProcessor invoked {"type":"reportColor","name":"electricPotentialLine","value":"#000000","phetioEventType":"wrapper"} 49 chargesAndFields.phetioCommandProcessor invoked {"type":"reportColor","name":"electricPotentialSensorCircleStroke","value":"#000000","phetioEventType":"wrapper"} 50 chargesAndFields.phetioCommandProcessor invoked {"type":"reportColor","name":"electricPotentialSensorCrosshairStroke","value":"#000000","phetioEventType":"wrapper"} 51 chargesAndFields.phetioCommandProcessor invoked {"type":"reportColor","name":"electricPotentialSensorTextPanelBorder","value":"#fafafa","phetioEventType":"wrapper"} 52 chargesAndFields.phetioCommandProcessor invoked {"type":"reportColor","name":"enclosureBorder","value":"#c0c0c0","phetioEventType":"wrapper"} 53 chargesAndFields.phetioCommandProcessor invoked {"type":"reportColor","name":"enclosureFill","value":"#eeeeee","phetioEventType":"wrapper"} 54 chargesAndFields.phetioCommandProcessor invoked {"type":"reportColor","name":"enclosureText","value":"#000000","phetioEventType":"wrapper"} 55 chargesAndFields.phetioCommandProcessor invoked {"type":"reportColor","name":"gridLengthScaleArrowFill","value":"#ff9900","phetioEventType":"wrapper"} 56 chargesAndFields.phetioCommandProcessor invoked {"type":"reportColor","name":"gridLengthScaleArrowStroke","value":"#ff0000","phetioEventType":"wrapper"} 57 chargesAndFields.phetioCommandProcessor invoked {"type":"reportColor","name":"gridStroke","value":"#ffcc33","phetioEventType":"wrapper"} 58 chargesAndFields.phetioCommandProcessor invoked {"type":"reportColor","name":"gridTextFill","value":"#000000","phetioEventType":"wrapper"} 59 chargesAndFields.phetioCommandProcessor invoked {"type":"reportColor","name":"measuringTapeText","value":"#000000","phetioEventType":"wrapper"} 60 chargesAndFields.phetioCommandProcessor invoked {"type":"reportColor","name":"reversedBackground","value":"#000000","phetioEventType":"wrapper"} 61 chargesAndFields.phetioCommandProcessor invoked {"type":"reportColor","name":"voltageLabel","value":"#000000","phetioEventType":"wrapper"} 62 chargesAndFields.phetioCommandProcessor invoked {"type":"reportColor","name":"voltageLabelBackground","value":"#ffffff","phetioEventType":"wrapper"}

We found that if parent is not another frame, then window.parent === window.
Disregarding the potentially buggy behavior in PhET-iO that allowed these color profile events to be picked up by the data stream, is this the best check to be used (14 times) across the project?

Do we want these postMessages potentially sending to their own frame?

[samreid]

To clarify, the code in question is:

      window.parent && window.parent.postMessage( JSON.stringify( {
        type: 'reportColor',
        name: key,
        value: '#' + hexColor
      } ), '*' );

It appears the intention was to send the message to another frame, not to itself. Skimming through other occurrences of window.parent && window.parent., they all seem to have the same intent.

MDN suggests using a check more like: https://developer.mozilla.org/en-US/docs/Web/API/Window/parent

if (window.parent != window.top) {
  // we're deeper than one down
}

But checking window.parent!==window also seems reasonable.

[jessegreenberg]

Both of those sound fine to me. It looks like we are already using window.parent !== window in a few places.

[samreid]

@samreid will review cases and convert logic for cases that are obviously not meant for self window. Where unclear, I'll reach out to responsible devs.

[samreid]

I reviewed each case and it seemed like they intended to send messages to a non-self window. I'll commit the MDN recommendation shortly.

[samreid]

Pushed, closing. Will post to slack and check CT to see if anything comes up.

Posted on slack:

I changed the logic about checking whether a sim is in an iframe in #958, please report any problems you observe.

[jessegreenberg]

In phetsims/a11y-research#126, it was found that the a11y view is no longer receiving the "load" message from the sim embedded in the iframe.

[jessegreenberg]

It seems like the check we want is window.top !== window. Or maybe window.parent !== window

[samreid]

My apologies, @jessegreenberg can you please determine the correct logic for that case? Also, can you comment on what makes that case unique? Or do we need to update all of these sites?

[jessegreenberg]

I inspected each case, I believe the intention for each is to send a message to the parent frame so window.parent !== window seems correct. I tested a11y-view and the color picker iframe tool, they seem to be working correctly.

LoL harness isn't working I can't tell if that is because of this change or something else, Ill keep looking.

EDIT: Chrome was blocking LoL harness, saying "Page is trying to load unsafe scripts". When I allowed it the sim ran correctly.

[jessegreenberg]

@samreid seem OK to reclose?

[samreid]

You're right, thanks for correcting this, closing.

[zepumph]

In the "allow postMessage to own frame" commit, I removed a few usages of window.parent !== window. There are still 12 more usages though. I wanted to change a few and then get a review before continuing. @samreid I'm happy to discuss further if needed. That said please review the changes in the aforementioned commit.

[samreid]

First, a minor point. It seems window.parent always exists, so window.parent && window.parent.postMessage can be changed to window.parent.postMessage. Second, I'm still unclear why binder requires a "same-frame" load message. If that is really what is happening, why can't binder use some other within-frame way of listening for when sim load is complete, such as endedSimConstructionEmitter?

[zepumph]

That's a good idea, and I can experiment with that, but the main use case is loading the sim from pupeteer, and then needing to know when the sim has loaded. I'm not sure if it will be easy to make that requirejs step has completed (such that there is Sim.endedSimConstructionEmitter). The Postmessage is a clean and nice way to handle this to me.

[zepumph]

I would like to bring this up for developer meeting. To me it doesn't seem like too big of a deal to send these messages to itself if window.parent === window. That said I would like to bring it up for developer meeting to see what potential problems there are with it.

[samreid]

My main question is whether it is an antipattern to send a within-frame message via postMessage rather than using a traditional function call. Or if that is an anti-pattern as a rule of thumb, if this case is an exception to that rule.

[jessegreenberg]

I don't see anything here (https://developer.mozilla.org/en-US/docs/Web/API/Window/postMessage) that discourages within-frame messages, but at the same all documentation is about cross window/frame communcation.

[samreid]

After discussion at the previous developer meeting, it seems the next step is for me to discuss this further with @zepumph before we bring it back to the group.

[samreid]

We have priorities at the moment, but I added a personal calendar notification for next week to try to schedule a time for discussion. Self-unassigning for now.