document.all() has stopped working again because JSClass changed and consumers were not updated

RESOLVED FIXED in Firefox 13

Status

()

Core
JavaScript Engine
--
major
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: sid0, Assigned: luke)

Tracking

({regression, testcase})

Trunk
mozilla15
regression, testcase
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox13+ verified, firefox14+ verified, firefox15+ verified)

Details

(Whiteboard: [qa+])

Attachments

(3 attachments, 1 obsolete attachment)

Created attachment 617192 [details]
test case

+++ This bug was initially created as a clone of Bug #626050 +++

document.all() has stopped working again. The attached test case works fine in IE9 and in Chrome 18.0.1025.162 on Windows 7, but not in Mozilla/5.0 (Windows NT 6.1; WOW64; rv:14.0) Gecko/20120420 Firefox/14.0a1.
(Reporter)

Updated

5 years ago
Severity: normal → critical
(Reporter)

Updated

5 years ago
Severity: critical → major
I'm just eyeballing code here.  But how in the world is this supposed to work?  Isn't document.all represented by an object of the sHTMLDocumentAllClass class in nsDOMClassInfo.cpp?  And doesn't that class have its .call = nsnull?  And as I look at hg annotate for the class, hasn't it had .call = nsnull back to hg@1?

Which I guess is my way of saying I really want to know a regression range here.  Because I don't see why this would ever have worked, if we were using that class all along for that object.
Created attachment 617195 [details] [diff] [review]
Patch?

This appears to fix the test case
The regression range is http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=be559203ece8&tochange=373c710112e6

Which means this is a bug in 13 and 14, but not 12, I would think.

And the reason this regressed is that JSClass changed in bug 726944.  Specifically, the "JSClassInternal     reserved0;" before the checkAccess hook got removed, which shifted all the later hooks.  And the result was that what used to be a call hook became a construct hook and what used to be a checkAccess hook became a call hook and what used to be null became a checkAccess hook.

document.all.tags got broken in the same way; yay lock of tests there.

So the good news is that non-null checkAccess hooks probably failed to compile when viewed as a call hook, so we don't have to worry about those cases.  So the only breakage should have been cases that had a null checkAccess hook and a non-null call hook and a null construct hook: that would have pushed null into call, call into construct, and null into hasInstance, which must already have been null for this to compile.
Blocks: 726944
tracking-firefox13: --- → ?
I think I agree with that diagnosis.

We agreed on IRC that given the branch tomorrow, the safe thing to do is just to ship aurora with what we have, then land the fix in both aurora and beta after the branch tomorrow.  It's a little unfortunate that we'll have to land twice, but it seems safer than doing a rush job now and finding out we screwed something up after releasing a broken alpha.  Also we'll get some data on how much document.all actually gets called in the real world -- helpful data concerning the possibility of getting rid of document.all.

I mentioned it on IRC, but if we enforced use of JSCLASS_NO_OPTIONAL_MEMBERS (which now no longer exists -- we removed it because we weren't enforcing its correct use, and we weren't using it regularly or consistently), the JSClass struct layout change would have caught this, by producing an initializer with excess initializers, clearly in need of fixing.  There's some gcc warning (that we could turn into an error) to require structs to be {}-initialized with their full complement of members specified, to ensure we used JSCLASS_NO_OPTIONAL_MEMBERS everywhere it needed to be used.  Whether this is even possible to do -- system headers might rely on zero-initialization of trailing members -- or even desirable, I dunno.  But it seems worth pointing out.
tracking-firefox13: ? → +
tracking-firefox14: ? → +
tracking-firefox15: --- → +
Assignee: general → nobody
Component: JavaScript Engine → DOM: Core & HTML
QA Contact: general → general
Not that this bug needs someone to go through the tree looking for the other places (if any) missed in bug 726944.
Per comment 5.
Assignee: nobody → general
Component: DOM: Core & HTML → JavaScript Engine
QA Contact: general → general
Summary: document.all() has stopped working again → document.all() has stopped working again because JSClass changed and consumers were not updated
Hey, what's supposed to happen next here? We've got a patch up but with no r?, and comment 4 and comment 5 give some next steps, but nothing's happened in the past week.
Well, ideally what happens is that the people who broke this step up and fix it...

If they don't plan to do that, then they need to say so, and then the next step is that I spend the time needed to sort through this and fix it.

The patch attached here is patently insufficient given comment 3: it doesn't fix all the things we already know are broken.
(In reply to Boris Zbarsky (:bz) from comment #8)
> Well, ideally what happens is that the people who broke this step up and fix
> it...
> 
> If they don't plan to do that, then they need to say so, and then the next
> step is that I spend the time needed to sort through this and fix it.
> 
> The patch attached here is patently insufficient given comment 3: it doesn't
> fix all the things we already know are broken.

Could we just add "JSClassInternal     reserved0;" back in?
If we also revert the corresponding changes to all the JSClasses tahat _did_ get changed, and audit whatever JSClasses have been added to the tree since then, then yes.
(In reply to Boris Zbarsky (:bz) from comment #10)
> If we also revert the corresponding changes to all the JSClasses tahat _did_
> get changed, and audit whatever JSClasses have been added to the tree since
> then, then yes.

What do you think will be easier: doing that, or the other way, which I assume is to audit all current JSClasses? The latter seems easier.
I really don't know.  Seems like either one would involve making some change to JSClass that makes them _all_ not compile so they can actually be found, and then fixing them one by one as needed.

Updated

5 years ago
Blocks: 752282
Oh, lovely.  The change that caused the problem never even got reviewed over in bug 726944 a far as I can tell.
(Assignee)

Comment 14

5 years ago
If Igor doesn't want to, I guess I should...
Assignee: general → luke
(Assignee)

Comment 15

5 years ago
Created attachment 621699 [details] [diff] [review]
fix and prevent silent errors

This patch switches 'hasInstance' with 'construct' so that off-by-one errors get a compile error.  This would have caught the two cases in nsDOMClassInfo mentioned earlier; it looks like there weren't any more.
Attachment #617195 - Attachment is obsolete: true
Attachment #621699 - Flags: review?(bzbarsky)
Comment on attachment 621699 [details] [diff] [review]
fix and prevent silent errors

r=me.  Thanks for picking this up!
Attachment #621699 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 17

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/7082192622e6
Target Milestone: --- → mozilla15
We should get this on all the branches too (though for branches it's probably better to not change JSClass and just change the two JSClasses involved here instead).
https://hg.mozilla.org/mozilla-central/rev/7082192622e6
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
(Assignee)

Comment 20

5 years ago
Created attachment 622405 [details] [diff] [review]
fix for aurora/beta

[Approval Request Comment]
Regression caused by (bug #): 726944
User impact if declined: document.all changes behavior
Testing completed (on m-c, etc.): m-c
Risk to taking this patch (and alternatives if risky): very low
Attachment #622405 - Flags: review?(bzbarsky)
Attachment #622405 - Flags: approval-mozilla-beta?
Attachment #622405 - Flags: approval-mozilla-aurora?
Comment on attachment 622405 [details] [diff] [review]
fix for aurora/beta

r=me
Attachment #622405 - Flags: review?(bzbarsky) → review+
Comment on attachment 622405 [details] [diff] [review]
fix for aurora/beta

[Triage Comment]
Low risk fix for a regression in FF13. Approved for Beta 13 and Aurora 14.
Attachment #622405 - Flags: approval-mozilla-beta?
Attachment #622405 - Flags: approval-mozilla-beta+
Attachment #622405 - Flags: approval-mozilla-aurora?
Attachment #622405 - Flags: approval-mozilla-aurora+
(Assignee)

Comment 23

5 years ago
https://hg.mozilla.org/releases/mozilla-aurora/rev/7ffd2eac063d
https://hg.mozilla.org/releases/mozilla-beta/rev/31261e28a239
(Assignee)

Updated

5 years ago
status-firefox13: --- → fixed
status-firefox14: --- → fixed
status-firefox15: --- → fixed
Keywords: testcase
Whiteboard: [qa+]
Duplicate of this bug: 752282
Verified that the document.all()works on Firefox 13 beta 4 using the test case attached in the Description.

Verified on Windows XP, Ubuntu 12.04 and Mac OS X 10.6:
Mozilla/5.0 (Windows NT 5.1; rv:13.0) Gecko/20100101 Firefox/13.0
Mozilla/5.0 (X11; Linux i686; rv:13.0) Gecko/20100101 Firefox/13.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:13.0) Gecko/20100101 Firefox/13.0
status-firefox13: fixed → verified
Verified on Firefox 14 beta 7  that the document.all() works again - used the test case attached in the Description. Verified on Win 7, Ubuntu 12.04 and Mac OS X 10.6:

Mozilla/5.0 (Windows NT 6.1; rv:14.0) Gecko/20100101 Firefox/14.0
Mozilla/5.0 (X11; Linux i686; rv:14.0) Gecko/20100101 Firefox/14.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:14.0) Gecko/20100101 Firefox/14.0
status-firefox14: fixed → verified
Verified on Firefox 15 beta 2 that the document.all() works again (verified on Windows 7, Ubuntu 12.04 and Mac OS X 10.6).

Mozilla/5.0 (Windows NT 6.1; rv:15.0) Gecko/20100101 Firefox/15.0
Mozilla/5.0 (X11; Linux i686; rv:15.0) Gecko/20100101 Firefox/15.0	
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:15.0) Gecko/20100101 Firefox/15.0
status-firefox15: fixed → verified
You need to log in before you can comment on or make changes to this bug.