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

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Rain [:rain1] (inactive)]

Attachment: 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.

Comment 1

[Jeff Walden [:Waldo]]

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.

Comment 2

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

Attachment: Patch?

This appears to fix the test case

Comment 3

[Boris Zbarsky [:bzbarsky]]

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.

Comment 4

[Jeff Walden [:Waldo]]

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.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Not that this bug needs someone to go through the tree looking for the other places (if any) missed in bug 726944.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Per comment 5.

Comment 7

[David Mandelin [:dmandelin]]

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.

Comment 8

[Boris Zbarsky [:bzbarsky]]

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.

Comment 9

[David Mandelin [:dmandelin]]

(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?

Comment 10

[Boris Zbarsky [:bzbarsky]]

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.

Comment 11

[David Mandelin [:dmandelin]]

(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.

Comment 12

[Boris Zbarsky [:bzbarsky]]

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.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Oh, lovely.  The change that caused the problem never even got reviewed over in bug 726944 a far as I can tell.

Comment 14

[Luke Wagner [:luke]]

If Igor doesn't want to, I guess I should...

Comment 15

[Luke Wagner [:luke]]

Attachment: 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.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 621699 [details] [diff] [review]
fix and prevent silent errors

r=me.  Thanks for picking this up!

Comment 17

[Luke Wagner [:luke]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7082192622e6

Comment 18

[Boris Zbarsky [:bzbarsky]]

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).

Comment 19

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/7082192622e6

Comment 20

[Luke Wagner [:luke]]

Attachment: 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

Comment 21

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 622405 [details] [diff] [review]
fix for aurora/beta

r=me

Comment 22

[Alex Keybl [:akeybl]]

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.

Comment 23

[Luke Wagner [:luke]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/7ffd2eac063d
https://hg.mozilla.org/releases/mozilla-beta/rev/31261e28a239

Comment 25

[Simona Badau, Desktop QA]

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

Comment 26

[Simona Badau, Desktop QA]

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

Comment 27

[Simona Badau, Desktop QA]

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