User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/60.0.3112.113 Safari/537.36 Steps to reproduce: Download/clone repository here: https://github.com/danbopes/firefox-addon-webcomponents-polyfill-crash and extract to a folder. Load the downloaded addon via temporary addon (about:debugging). Load http://example.com/ Actual results: Tab crashed: https://i.imgur.com/xHEnY6Y.png Expected results: Tab shouldn't have crashed.

Can you attach a link to the crash report from about:crashes?

Never mind: https://crash-stats.mozilla.com/report/index/dd9a7e32-10b2-4d6d-a3ea-cf29e0170904

When we have a property descriptor with only a setter or only a getter, we set both setter and getter flags, but leave set the value for the missing operation to undefined. XrayWrapper code currently assumes that if we have the flag, we have a function: http://searchfox.org/mozilla-central/rev/f2a1911ad310bf8651f342d719e4f4ca0a7b9bfb/js/xpconnect/wrappers/XrayWrapper.cpp#2222-2227 Which means that we crash when trying to defineProperty over a readonly attribute on an X-ray wrapper.

Comment on attachment 8904384 [details] [diff] [review] Bug 1396570 Review of attachment 8904384 [details] [diff] [review]: ----------------------------------------------------------------- Fine by me, but check with Jan that this is the correct contract.

(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #5) > Fine by me, but check with Jan that this is the correct contract. This contract is asserted in several places, and documented here: http://searchfox.org/mozilla-central/rev/f2a1911ad310bf8651f342d719e4f4ca0a7b9bfb/js/src/vm/NativeObject.cpp#2061-2068

Comment on attachment 8904384 [details] [diff] [review] Bug 1396570 Review of attachment 8904384 [details] [diff] [review]: ----------------------------------------------------------------- LGTM. (Killing off JSPROP_GETTER/JSPROP_SETTER and the ugly JSObject* <-> JSPropertyOp casts is on my list for post-57.)

(This is just a nullptr crash, right? In that case we can open this up.)

(In reply to Jan de Mooij [:jandem] from comment #8) > (This is just a nullptr crash, right? In that case we can open this up.) It is, yes. It's probably not exploitable in any interesting way, but it can probably be triggered by web content that knows it's being accessed through X-rays, so I figured I'd err on the safe side.

https://hg.mozilla.org/integration/mozilla-inbound/rev/1fc9e24649a8971d09ab87824478f6fd58b1c0e8 Bug 1396570: Null check getter/setter when JSPROP_GETTER/JSPROP_SETTER is set. r=bholley f=jandem

Comment on attachment 8904384 [details] [diff] [review] Bug 1396570 Approval Request Comment [Feature/Bug causing the regression]: Bug 872772 [User impact if declined]: This bug causes crashes in certain circumstances when privileged JavaScript interacts with content JavaScript, particularly in the case of extension content scripts. [Is this code covered by automated tests?]: The security wrappers in general are, though there isn't coverage for this particular crash. [Has the fix been verified in Nightly?]: No [Needs manual test from QE? If yes, steps to reproduce]: Yes. STR in comment 0 [List of other uplifts needed for the feature/fix]: None [Is the change risky?]: No [Why is the change risky/not risky?]: It simply adds a null check in a place where the only possible effect of a null value is a crash. [String changes made/needed]: None

Hey guys, just want to say thanks for the quick fix. I'm looking at implementation of an addon that requires ShadowDOM support, so the polyfill is going to need to be required. Is there a possible workaround to allow me to import the polyfill (Perhaps removing a few lines that cause the issue), so that current versions aren't affected by this crash?

Hi danbopes, Can you help check if this issue was fixed in the latest nightly? Thanks.

Comment on attachment 8904384 [details] [diff] [review] Bug 1396570 Add a null check. Let's take this. Beta56+.

Comment on attachment 8904384 [details] [diff] [review] Bug 1396570 add null check, esr52.4+

(In reply to Gerry Chang [:gchang] from comment #14) > Hi danbopes, > Can you help check if this issue was fixed in the latest nightly? Thanks. I'm unable to confirm this on nightly. I wasn't able to reproduce the crash, but I also wasn't able to confirm that the file was actually being loaded fully. Adding "console.log('webcomponents test');" failed to echo to the console, which tells me at some point something failed.

Reproduced the initial issue using Nightly 57.0a1 (2017-09-04). Investigated that the crash is not reproducible using the latest Nightly 57.0a1(2017-09-20) on Windows 10 x64, macOS 10.12 and Ubuntu 16.04 64bit. Similar to comment 19: in Browser Console I get the following Error: TypeError: "importNode" is read-only webcomponents-lite.js:82:120, with the note that I didn't see this error on affected build (2017-09-04): which makes me believe as well, that the file might not be fully loaded (comment 19). Kris, could you please take a look?

(In reply to roxana.leitan@softvision.ro from comment #20) > Similar to comment 19: in Browser Console I get the following Error: > TypeError: "importNode" is read-only webcomponents-lite.js:82:120, with the > note that I didn't see this error on affected build (2017-09-04): which > makes me believe as well, that the file might not be fully loaded (comment > 19). That's a separate bug, if it's a bug at all. The reason you don't see that error in the affected build is that it crashes before it ever gets to that point.

Verified fixed using Nightly 57.0a1(2017-09-20) on Windows 10 x64, macOS 10.12 and Ubuntu 16.04 64bit.

I reproduced this issue using Fx 57.0a1, build ID: 20170904220027, on Windows 10 x64. I can confirm this issue is fixed, I verified using Fx 56.0, build ID: 20170922200134, on Windows 10 x64, macOS X 10.12.6 and Ubuntu 14.04 LTS. Cheers!