Closed Bug 916644 Opened 10 years ago Closed 9 years ago

Disallow calling WebIDL constructors as functions on the web

Categories

(Core :: DOM: Core & HTML, defect)

Product:
Core
Component:
DOM: Core & HTML
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla30
Tracking Flags:
Tracking Status
relnote-firefox --- 30+

People

(Reporter: bzbarsky, Assigned: bholley)

References

Details

(Keywords: dev-doc-complete, site-compat)

Attachments

(1 file, 2 obsolete files)

Disable invoking WebIDL constructors without |new| unless you have the system principal. v2
2.85 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review 
See https://www.w3.org/Bugs/Public/show_bug.cgi?id=22808

We can't just do this: our chrome uses this all over.  Including the addon SDK.  A try push with this change is beautifully orange.

So I'm going to see if we can disable this for non-chrome callers only, for now and then get our tests passing.  And if we can, I will then reenable it for non-chrome code, add telemetry, and see what we see in the wild.
https://tbpl.mozilla.org/?tree=Try&rev=f2d188af713c says failures in M1, M2, M3, M5, mochitest-browser-chrome, mochitest-chrome, crashtest, crashtest-ipc, jetpack tests.  And probably the robocop tests (rc2) on Android 2.2.
Attachment #8378065 - Attachment is obsolete: true
Assignee: nobody → bobbyholley
Depends on: 976105
Summary: Measure whether we can disallow calling WebIDL constructors as functions → Disallow calling WebIDL constructors as functions on the web
I DeMorgan-ified the previous patch because I was finding it very difficult to
grok, even though I understood the change it was making.
Attachment #8381519 - Flags: review?(bzbarsky)
Attachment #8378066 - Attachment is obsolete: true
Comment on attachment 8381519 [details] [diff] [review]
Disable invoking WebIDL constructors without |new| unless you have the system principal. v2

r=me, but before you can land this you need to fix the gaia code that depends on the current behavior, no?

At the very least, http://mxr.mozilla.org/gaia/search?string=TextEncoder%28&find=&findi=&filter=^[^\0]*%24&hitlimit=&tree=gaia shows hits in the mail app, and similar for TextDecoder.  Not sure about other constructors that are worth checking for (e.g. Event() is really hard to search for).

If this passed tests, that mostly tells us that b2g test coverage is terrible, which is not a surprise.  :(
Attachment #8381519 - Flags: review?(bzbarsky) → review+
(In reply to Boris Zbarsky [:bz] (reviews will be slow; ask someone else) from comment #8)
> Comment on attachment 8381519 [details] [diff] [review]
> Disable invoking WebIDL constructors without |new| unless you have the
> system principal. v2
> 
> r=me, but before you can land this you need to fix the gaia code that
> depends on the current behavior, no?

Yeah. The try push in comment 7 is orange on Gaia, though probably not as orange as it should be. I'm looking into it, but it's all bundled up in sinon.js stuff.
Depends on: 976935
Depends on: 976937
Try push with a custom gaia including my changes from the dependent bugs:

https://tbpl.mozilla.org/?tree=Try&rev=0bf700091431
(In reply to Bobby Holley (:bholley) from comment #12)
> https://tbpl.mozilla.org/?tree=Try&rev=05180848d19c

The gaia stuff here is green with my patches.
Gaia changes should be merged in now. Try push without custom gaia:

https://tbpl.mozilla.org/?tree=Try&rev=6831a6cfbebc
Looks like Gaia bumps don't git m-c as frequently as I'd thought. Should be there by now:

https://tbpl.mozilla.org/?tree=Try&rev=33eb326fcc42
https://hg.mozilla.org/mozilla-central/rev/de377011ed9d
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
We probably want a followup for fixing the system-caller behavior too...
Blocks: 979591
Blocks: 981036
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.