694775 - Crash [@ nsComputedDOMStyle::GetPropertyCSSValue]

Status: RESOLVED WORKSFORME

(Core :: CSS Parsing and Computation, defect)

Description

[Jesse Ruderman]

Attachment: testcase (crashes Firefox when loaded)

The crash only happens on Windows (I don't know why), and only in debug builds (because http://hg.mozilla.org/mozilla-central/file/3b58a9df4c8c/layout/style/nsComputedDOMStyle.cpp#l514 is debug-only code).

Comment 1

[Jesse Ruderman]

Attachment: stack trace

Comment 2

[Boris Zbarsky [:bzbarsky]]

That's bizarre. The document proto should so not matter to this code...

Comment 3

[Scoobidiver (away)]

No crash for me on Windows 7.

Comment 4

[Jesse Ruderman]

[21:45:52.837] TypeError: anonymous method called on incompatible Proxy @ file:///C:/Users/jruderman/Desktop/b.html:5

Looks like it's no longer possible to set document.__proto__.  Is that intentional?

Comment 5

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Sounds like something to do with HTMLDocument-on-WebIDL.

Comment 6

[Peter Van der Beken [:peterv]]

I think that might be a result of bug 770344. We might be able to support this (treating it as an expando on setting), but it'll need JS engine changes afaict.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Er, wait.  Why does the JS engine not allow setting __proto__ on proxies, exactly?

Comment 8

[Boris Zbarsky [:bzbarsky]]

We should probably spin off a separate bug on making __proto__ work here again, then revisit this one once that's fixed...

Comment 9

[Jeff Walden [:Waldo]]

It's up to the proxy to make [[Prototype]] setting work.  Doing so requires overriding the proxy nativeCall trap.  Right now we've done it for the inside-the-engine proxy handlers (for new Proxy(), Proxy.create(), etc.) and for the wrapper classes that are basically engine-internal, but we haven't touched anything else.  If you want your proxies to allow this stuff, you need to add that trap and implement it to allow the mutation attempt to proceed.

Comment 10

[Jesse Ruderman]

Do we actually want to allow it?

Comment 11

[Boris Zbarsky [:bzbarsky]]

Jesse: yes, since ES is adding proto-mutation in a standard way.

Jeff: what does "allow the mutation attempt to proceed" mean, exactly?  How should an object that happens to be a proxy merely as an implementation detail go about mutating its proto?

Comment 12

[Boris Zbarsky [:bzbarsky]]

Actually, I spun off the __proto__ thing into bug 889198.

Comment 13

[Daniel Brooks [:db48x]]

Attachment: MozReview Request: Bug 694775 - avoid crashing by simply not climbing past the top of the tree (debug only)

Bug 694775 - avoid crashing by simply not climbing past the top of the tree (debug only)

I've recently tripped across this one on live webpages while trying to debug other things.

Comment 14

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8673967 [details]
MozReview Request: Bug 694775 - avoid crashing by simply not climbing past the top of the tree (debug only)

https://reviewboard.mozilla.org/r/22165/#review19763

OK, I guess.  We'll still assert if that root style context isn't for a pseudo-element.  And I think there are some obscure cases where we have style contexts that don't chain back up to the root element, where I suppose that's not a crazy thing to happen.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

FWIW, the attached testcase doesn't crash even without this patch applied. Would be nice if we could get one that still does.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Hrm, it didn't crash locally for me (using a trunk win64 debug build), but it does crash on Try still.

https://treeherder.mozilla.org/logviewer.html#?job_id=13301032&repo=try

Comment 17

[Ryan VanderMeulen [:RyanVM]]

To be clear, the crashing Try link above includes the patch from this bug.

Comment 18

[Daniel Brooks [:db48x]]

Nice. That's a slightly different stack so the patch is apparently just incomplete. I'll look into it.

I'll also try the testcase in the environment where I first saw this crash.

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d4d6f3565400
Add a crashtest.  r=mats

Comment 20

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/d4d6f3565400