1014258 - JSEventHandler CanSkip isn't called usefully

Status: VERIFIED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Olli Pettay [:smaug][[e-mail]]]

Attachment: patch

Because JSEventHandler isn't a JSHolder anymore, it's CanSkip isn't called always, which
means it doesn't end up calling mTarget->CanSkip.
This leads to large unoptimized-out areas in the CC graphs.
Test case is to load
http://www.whatwg.org/specs/web-apps/current-work/multipage/association-of-controls-and-forms.html#autofilling-form-controls:-the-autocomplete-attribute 

When marking JS areas black we should propagate blackness to C++ using CanSkip calls, but
that is perhaps a bit tricky to implement. So, for now, fix this regression.

https://tbpl.mozilla.org/?tree=Try&rev=9316901e19c1

Comment 1

[Andrew McCreight [:mccr8]]

Comment on attachment 8426567 [details] [diff] [review]
patch

Review of attachment 8426567 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/events/JSEventHandler.cpp
@@ +35,5 @@
>    , mTypedHandler(aTypedHandler)
>  {
>    nsCOMPtr<nsISupports> base = do_QueryInterface(aTarget);
>    mTarget = base.get();
> +  // Note, we call HoldJSObjects to get CanSkip called before CC.

nit: "Note:" rather than "Note," maybe?

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/da6db31fc9c4

Nah, ',' it is. ':' is stronger, and it doesn't need to be.
(And I so use Finnish rules for punctuation marks.)

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8426567 [details] [diff] [review]
patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 986304
User impact if declined: possibly higher cycle collection times
Testing completed (on m-c, etc.): just landed to m-i
Risk to taking this patch (and alternatives if risky): not risky 
String or IDL/UUID changes made by this patch: NA

Comment 4

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 5

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/a0779ff10844

Comment 6

[u279076]

(In reply to Olli Pettay [:smaug] from comment #0)
> Test case is to load
> http://www.whatwg.org/specs/web-apps/current-work/multipage/association-of-
> controls-and-forms.html#autofilling-form-controls:-the-autocomplete-
> attribute 

To verify this is fixed, what am I looking for on this page?

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

Need to check cycle collector graph.
One way is to install the addon attached to https://bugzilla.mozilla.org/show_bug.cgi?id=726346, 
restart browser, load that whatwg page in one tab and then about:cc in another one.
Run cycle collector and ensure the graph is not huge (<10000 objects).

But I guess I could verify this.

Comment 8

[Henrik Skupin [:whimboo][⌚️UTC+1]]

The scenario for testing as pointed out in comment 7 sounds complex, and the code impact seems to be small here. Given the amount of other important test cases we have to add, I don't think we should do more as the manual verification here. Olli, what do you think?

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Yeah, no need manual verification here (I've tested this).
(But I and mccr8 should figure out how to make sure we don't regress CC optimizations - some kind
 of test which analyzes non-allTraces CC logs after running testcases.)

Comment 10

[Henrik Skupin [:whimboo][⌚️UTC+1]]

So this will be a test you will/can take care of, right? Is there a framework, which can be used for that? Or is there really nothing appropriate for an in-tree test?

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

There isn't really any framework. We have leak testing which checks if CC graph has Window objects, but
we need some new framework where one can easily check existence of any type of object in CC graph.

Comment 12

[Henrik Skupin [:whimboo][⌚️UTC+1]]

So maybe filing such a bug and marking possible bugs with test requests (like this one) as dependent on it? Otherwise we may loose possible tests.

Comment 13

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

(In reply to Olli Pettay [:smaug] from comment #9)
> Yeah, no need manual verification here (I've tested this).

Olli, did you verify this in both Firefox 31 and Firefox 32? If so we can mark this as Verified.

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

I actually didn't verify this on 31.
Let me do that now.

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

I see now similar behavior on 31 and trunk.

Comment 16

[u279076]

Thanks for your help, Olli.