Last Comment Bug 747617 - document.all() has stopped working again because JSClass changed and consumers were not updated
: document.all() has stopped working again because JSClass changed and consumer...
Status: RESOLVED FIXED
[qa+]
: regression, testcase
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: -- major (vote)
: mozilla15
Assigned To: Luke Wagner [:luke]
:
Mentors:
: 752282 (view as bug list)
Depends on: 626050
Blocks: 726944 752282
  Show dependency treegraph
 
Reported: 2012-04-21 01:06 PDT by Siddharth Agarwal [:sid0] (inactive)
Modified: 2012-07-27 08:30 PDT (History)
23 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
verified
+
verified
+
verified


Attachments
test case (185 bytes, text/html)
2012-04-21 01:06 PDT, Siddharth Agarwal [:sid0] (inactive)
no flags Details
Patch? (1.53 KB, patch)
2012-04-21 02:44 PDT, :Ms2ger (⌚ UTC+1/+2)
no flags Details | Diff | Splinter Review
fix and prevent silent errors (17.12 KB, patch)
2012-05-07 12:58 PDT, Luke Wagner [:luke]
bzbarsky: review+
Details | Diff | Splinter Review
fix for aurora/beta (1.77 KB, patch)
2012-05-09 10:04 PDT, Luke Wagner [:luke]
bzbarsky: review+
akeybl: approval‑mozilla‑aurora+
akeybl: approval‑mozilla‑beta+
Details | Diff | Splinter Review

Description Siddharth Agarwal [:sid0] (inactive) 2012-04-21 01:06:33 PDT
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.
Comment 1 Jeff Walden [:Waldo] (remove +bmo to email) 2012-04-21 02:34:22 PDT
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 (⌚ UTC+1/+2) 2012-04-21 02:44:30 PDT
Created attachment 617195 [details] [diff] [review]
Patch?

This appears to fix the test case
Comment 3 Boris Zbarsky [:bz] 2012-04-23 14:16:11 PDT
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] (remove +bmo to email) 2012-04-23 14:44:53 PDT
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 [:bz] 2012-04-23 18:27:52 PDT
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 [:bz] 2012-04-25 20:28:52 PDT
Per comment 5.
Comment 7 David Mandelin [:dmandelin] 2012-05-02 17:36:16 PDT
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 [:bz] 2012-05-02 17:46:04 PDT
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] 2012-05-04 17:33:53 PDT
(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 [:bz] 2012-05-04 17:39:48 PDT
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] 2012-05-04 17:58:34 PDT
(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 [:bz] 2012-05-04 18:03:36 PDT
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 [:bz] 2012-05-07 09:46:38 PDT
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] 2012-05-07 09:55:34 PDT
If Igor doesn't want to, I guess I should...
Comment 15 Luke Wagner [:luke] 2012-05-07 12:58:05 PDT
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.
Comment 16 Boris Zbarsky [:bz] 2012-05-07 22:15:58 PDT
Comment on attachment 621699 [details] [diff] [review]
fix and prevent silent errors

r=me.  Thanks for picking this up!
Comment 18 Boris Zbarsky [:bz] 2012-05-08 17:37:06 PDT
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] 2012-05-09 03:42:57 PDT
https://hg.mozilla.org/mozilla-central/rev/7082192622e6
Comment 20 Luke Wagner [:luke] 2012-05-09 10:04:25 PDT
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
Comment 21 Boris Zbarsky [:bz] 2012-05-09 10:08:26 PDT
Comment on attachment 622405 [details] [diff] [review]
fix for aurora/beta

r=me
Comment 22 Alex Keybl [:akeybl] 2012-05-09 15:36:51 PDT
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 24 Naveed Ihsanullah [:naveed] 2012-05-11 11:39:16 PDT
*** Bug 752282 has been marked as a duplicate of this bug. ***
Comment 25 Simona B [:simonab ] -PTO- back Sept 5th 2012-05-17 08:33:18 PDT
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 B [:simonab ] -PTO- back Sept 5th 2012-06-14 07:51:36 PDT
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 B [:simonab ] -PTO- back Sept 5th 2012-07-27 08:30:37 PDT
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

Note You need to log in before you can comment on or make changes to this bug.