Closed Bug 601168 Opened 9 years ago Closed 9 years ago

nsHTMLPluginObjElementSH assumes |this| has been converted to object

Categories

(Core :: Plug-ins, defect)

defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: jimb, Unassigned)

References

Details

(Whiteboard: [fixed-in-tracemonkey])

Attachments

(1 file)

Under the new ES5 |this| regimen, null and undefined |this| values are converted to the global object by the callee as needed, not eagerly before the call by the caller's code.

However, nsHTMLPluginObjElementSH assumes that |this| is not null or undefined. Applying a plugin element as a function (say, |document.getElementById("plugin1")(42)|, something we support) causes an assertion with the strict |this| patches applied, as direct calls get |undefined| as their |this| value, and nsHTMLPlugiObjElementSH::Call tries to apply JSVAL_TO_OBJECT to that.

Steps to reproduce: run mochitest-plain's modules/plugin/test/test_npruntime_npninvokedefault.html with the strict |this| patches applied.
Attachment #480152 - Flags: review?(jst)
Comment on attachment 480152 [details] [diff] [review]
Allow nsHTMLPluginObjElementSH::Call to pass through non-Object |this| values to the plugin.

Johnny, if you could review the non-js/src parts of this, that would be great. The Call::JS interface stuff I'll have Andreas look at, as he's the one who suggested we get started populating the JS namespace with a properly C++-ish interface.
Attachment #480152 - Flags: review?(gal)
BTW: this blocks bug 514570, strict |this|.
gal, jst: I can't land strict |this| until this is reviewed. Please take a look at it as soon as you can.
Attachment #480152 - Flags: review?(jst) → review?(bzbarsky)
Comment on attachment 480152 [details] [diff] [review]
Allow nsHTMLPluginObjElementSH::Call to pass through non-Object |this| values to the plugin.

r=me on the dom bits
Attachment #480152 - Flags: review?(bzbarsky) → review+
Comment on attachment 480152 [details] [diff] [review]
Allow nsHTMLPluginObjElementSH::Call to pass through non-Object |this| values to the plugin.

I like it. Make sure brendan is CC and gets a chance to object if he wants since this is a public API but this looks pretty good to me.
Attachment #480152 - Flags: review?(gal) → review+
http://hg.mozilla.org/tracemonkey/rev/8e418df1af74
Will revise instantly if brendan has comments.
Whiteboard: [fixed-in-tracemonkey]
I'm really looking for a C++ API design lead, who will rule righteously. I'm not that guy. Putting C-like functions in namespace JS seems not as C++-l33t as some folks might want, and doing it in jsapi.h may limit what this "good start" could grow to become.

Maybe it's ok, it is a small patch. It doesn't really hurt. But should we want a more modern C++ API (in a separate .h file)? Cc'ing Jason and Luke for thoughts.

/be
http://hg.mozilla.org/mozilla-central/rev/8e418df1af74
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.