Last Comment Bug 718340 - Don't traverse black windows
: Don't traverse black windows
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM (show other bugs)
: 12 Branch
: All All
: -- normal (vote)
: ---
Assigned To: Olli Pettay [:smaug] (high review load, please consider other reviewers)
:
Mentors:
Depends on:
Blocks: 716598
  Show dependency treegraph
 
Reported: 2012-01-15 14:52 PST by Olli Pettay [:smaug] (high review load, please consider other reviewers)
Modified: 2012-01-17 23:30 PST (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (999 bytes, patch)
2012-01-15 14:52 PST, Olli Pettay [:smaug] (high review load, please consider other reviewers)
continuation: review+
jst: review+
Details | Diff | Review

Description Olli Pettay [:smaug] (high review load, please consider other reviewers) 2012-01-15 14:52:50 PST
Created attachment 588779 [details] [diff] [review]
patch

There are cases when window is still black, but document isn't in CC generation.

I think this would be the fix for jst's Google Reader problem.

I'd like to land this small patch separately so that possible regressions are easier
to find.
Comment 1 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2012-01-15 14:57:09 PST
The patch has been part of my bigger patch for ~two weeks now, and so far everything looks ok.
Comment 2 Andrew McCreight (PTO-ish through 6-29) [:mccr8] 2012-01-15 16:04:27 PST
Comment on attachment 588779 [details] [diff] [review]
patch

Makes sense.  The wrapper will always keep the window alive, so this makes sense to me.  Just out of curiousity, have you looked at doing this for nsGenericElements with wrappers?
Comment 3 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2012-01-15 16:10:45 PST
Yes, nodes do check blackness already. It is just that any changes to window's CC handling *may*
have more dramatic effects than changing just node/element/document handling - that is why
I want to land this small patch separately.
Comment 4 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2012-01-16 03:14:02 PST
https://hg.mozilla.org/mozilla-central/rev/93d7303c1684
Comment 5 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2012-01-16 08:34:10 PST
Some unknown orange. filed bug 716598
https://hg.mozilla.org/mozilla-central/rev/b3b8d62e0d92
Comment 6 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2012-01-16 08:34:54 PST
(In reply to Olli Pettay [:smaug] from comment #5)
> Some unknown orange. filed bug 716598
er, bug 718411
Comment 7 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2012-01-16 08:39:15 PST
I haven't seen the error on tryserver. Is it required to traverse timeouts even when
the window is black. But if so, why not when the document is in cc generation.
Comment 8 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2012-01-16 12:14:39 PST
I don't understand it. Timeouts should be traversed anyway, since they are stored in mJSHolders
and XPConnect should add that stuff to CC.
Comment 9 Andrew McCreight (PTO-ish through 6-29) [:mccr8] 2012-01-16 12:49:43 PST
Have you tried turning on CC logging and seeing what they look like with and without this turned optimization?
Comment 10 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2012-01-16 12:56:28 PST
Well, there is certainly a bug in videocontrols.xml.

But no, I haven't checked the CC logs, since I can't reproduce the problem.
Comment 11 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2012-01-16 14:17:06 PST
And looks like the failure isn't caused by this. It happened even after backout.
Comment 12 Andrew McCreight (PTO-ish through 6-29) [:mccr8] 2012-01-16 14:37:42 PST
Well, at least that means we still probably understand what is happening.
Comment 13 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2012-01-17 02:50:39 PST
https://hg.mozilla.org/mozilla-central/rev/0e7d183d0d12

The problem ended up being a regression from another patch which landed just before this one.

Hopefully better luck this time.

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