Last Comment Bug 720799 - Don't use mDocShell in nsScreen
: Don't use mDocShell in nsScreen
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla14
Assigned To: Mounir Lamouri (:mounir)
:
:
Mentors:
Depends on: 745495
Blocks: 720794
  Show dependency treegraph
 
Reported: 2012-01-24 12:35 PST by Mounir Lamouri (:mounir)
Modified: 2012-04-14 17:45 PDT (History)
7 users (show)
mounir: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (5.44 KB, patch)
2012-01-24 12:35 PST, Mounir Lamouri (:mounir)
jst: review+
mounir: checkin-
Details | Diff | Splinter Review
Update to current tip (3.89 KB, patch)
2012-03-14 09:36 PDT, Mounir Lamouri (:mounir)
justin.lebar+bug: review-
Details | Diff | Splinter Review
Update to current tip (4.33 KB, patch)
2012-03-14 09:57 PDT, Mounir Lamouri (:mounir)
justin.lebar+bug: review+
Details | Diff | Splinter Review

Description Mounir Lamouri (:mounir) 2012-01-24 12:35:20 PST
Created attachment 591223 [details] [diff] [review]
Patch
Comment 1 Johnny Stenback (:jst, jst@mozilla.com) 2012-01-24 14:55:29 PST
Comment on attachment 591223 [details] [diff] [review]
Patch

-  screen->mDocShell = aWindow->GetDocShell();
+  screen->mIsChrome = IsChromeType(aWindow->GetDocShell);

r=jst if you make that line actually compile :)

The rest looks good!
Comment 2 Mounir Lamouri (:mounir) 2012-02-09 02:39:05 PST
Comment on attachment 591223 [details] [diff] [review]
Patch

Pushed with a compiling patch :)
Comment 3 Ed Morley [:emorley] 2012-02-09 04:10:40 PST
Backed out of inbound since it conflicted with the prior push, which failed to build on all platforms:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=4a763183482f

https://hg.mozilla.org/integration/mozilla-inbound/rev/c4c7eaaeec91
Comment 4 Ed Morley [:emorley] 2012-02-09 08:45:52 PST
Relanded, but the push had to be backed out again for bustage:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=1ca8d5a931ac

https://hg.mozilla.org/integration/mozilla-inbound/rev/33ffd55f2dfe

Please can this/the rest of the push be run on try before landing a third time :-)
Comment 5 Mounir Lamouri (:mounir) 2012-03-14 09:36:53 PDT
Created attachment 605802 [details] [diff] [review]
Update to current tip

Justin did some changes that broke that patch... So I guess he will be glad to review the changes I had to do :)
Comment 6 Justin Lebar (not reading bugmail) 2012-03-14 09:45:03 PDT
(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #5)
> Justin did some changes that broke that patch... So I guess he will be glad
> to review the changes I had to do :)

Sure!  But which changes broke you, exactly?  The last time I touched nsScreen.cpp was Dec 5.
Comment 7 Justin Lebar (not reading bugmail) 2012-03-14 09:47:20 PDT
Maybe it's bug 729430 (which I reviewed)?
Comment 8 Mounir Lamouri (:mounir) 2012-03-14 09:49:19 PDT
Yes, indeed. You did review that bug so you can obviously review this one :)
Comment 9 Justin Lebar (not reading bugmail) 2012-03-14 09:52:14 PDT
Comment on attachment 605802 [details] [diff] [review]
Update to current tip

> +  if (!sAllowScreenEnabledProperty || !(mIsChrome || IsWhiteListed(GetOwner()))) {

Would you mind changing this to

 if (!sAllowScreenEnabledProperty || IsWhiteListed())

and making IsWhiteListed a member function which calls GetOwner() and checks mIsChrome?
Comment 10 Mounir Lamouri (:mounir) 2012-03-14 09:57:13 PDT
Created attachment 605812 [details] [diff] [review]
Update to current tip
Comment 11 Justin Lebar (not reading bugmail) 2012-03-14 10:06:30 PDT
> +  nsCOMPtr<nsIDocument> doc = do_GetInterface(GetOwner()->GetDocShell());
> +  nsIPrincipal *principal = doc->NodePrincipal();

doc can't be null?
Comment 12 Mounir Lamouri (:mounir) 2012-03-14 11:09:38 PDT
(In reply to Justin Lebar [:jlebar] from comment #11)
> > +  nsCOMPtr<nsIDocument> doc = do_GetInterface(GetOwner()->GetDocShell());
> > +  nsIPrincipal *principal = doc->NodePrincipal();
> 
> doc can't be null?

The code I'm moving is assuming that. Did you review the first patch without checking? :)
Comment 13 Justin Lebar (not reading bugmail) 2012-03-14 11:11:41 PDT
(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #12)
> (In reply to Justin Lebar [:jlebar] from comment #11)
> > > +  nsCOMPtr<nsIDocument> doc = do_GetInterface(GetOwner()->GetDocShell());
> > > +  nsIPrincipal *principal = doc->NodePrincipal();
> > 
> > doc can't be null?
> 
> The code I'm moving is assuming that. Did you review the first patch without
> checking? :)

So it would seem...
Comment 14 Mounir Lamouri (:mounir) 2012-03-14 11:13:59 PDT
I've added a null-check because it doesn't seem obvious that |doc| can never be null. Even |GetDocShell()| isn't guaranteed to not return null and the QI isn't guaranteed to succeed. Maybe it will always work in practice but better to be in the safe side here.
Comment 15 Phil Ringnalda (:philor) 2012-03-17 17:13:22 PDT
https://hg.mozilla.org/mozilla-central/rev/38a71ae7e06d
Comment 16 Mark Finkle (:mfinkle) (use needinfo?) 2012-03-17 19:00:52 PDT
I backed this out along with all the other patches in the initial push. Something in the push regressed Ts on Android by 20% and I don't know which patch is to blame:

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

Regression Ts increase 20.6% on Android 2.2 Mozilla-Inbound
--------------------------------------------------------------
    Previous: avg 2653.656 stddev 87.856 of 30 runs up to revision 1239bd0779a6
    New     : avg 3201.178 stddev 110.202 of 5 runs since revision 0d61a0d8dba4
    Change  : +547.522 (20.6% / z=6.232)
    Graph   : http://mzl.la/zD3EWy
Comment 17 Marco Bonardo [::mak] 2012-03-20 03:48:18 PDT
https://hg.mozilla.org/mozilla-central/rev/64c0ad726111

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