Closed Bug 694775 Opened 8 years ago Closed Last year

Crash [@ nsComputedDOMStyle::GetPropertyCSSValue]

Categories

(Core :: CSS Parsing and Computation, defect, critical)

x86_64
Windows 7
defect
Not set
critical

Tracking

()

RESOLVED WORKSFORME

People

(Reporter: jruderman, Assigned: db48x)

References

(Blocks 2 open bugs)

Details

(Keywords: crash, testcase)

Crash Data

Attachments

(3 files)

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).
Attached file stack trace
That's bizarre. The document proto should so not matter to this code...
No crash for me on Windows 7.
[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?
Sounds like something to do with HTMLDocument-on-WebIDL.
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.
Er, wait.  Why does the JS engine not allow setting __proto__ on proxies, exactly?
Flags: needinfo?(jwalden+bmo)
We should probably spin off a separate bug on making __proto__ work here again, then revisit this one once that's fixed...
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.
Flags: needinfo?(jwalden+bmo)
Do we actually want to allow it?
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?
Flags: needinfo?(jwalden+bmo)
Actually, I spun off the __proto__ thing into bug 889198.
Depends on: 889198
Flags: needinfo?(jwalden+bmo)
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.
Assignee: nobody → db48x
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.
Attachment #8673967 - Flags: review+
Keywords: checkin-needed
FWIW, the attached testcase doesn't crash even without this patch applied. Would be nice if we could get one that still does.
Flags: in-testsuite?
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
Flags: in-testsuite? → in-testsuite+
Keywords: checkin-needed
To be clear, the crashing Try link above includes the patch from this bug.
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.
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.