The default bug view has changed. See this FAQ.

Don't use mDocShell in nsScreen

RESOLVED FIXED in mozilla14

Status

()

Core
DOM
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: mounir, Assigned: mounir)

Tracking

Trunk
mozilla14
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

5.44 KB, patch
jst
: review+
mounir
: checkin-
Details | Diff | Splinter Review
4.33 KB, patch
Justin Lebar (not reading bugmail)
: review+
Details | Diff | Splinter Review
(Assignee)

Description

5 years ago
Created attachment 591223 [details] [diff] [review]
Patch
Attachment #591223 - Flags: review?(jst)
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!
Attachment #591223 - Flags: review?(jst) → review+
(Assignee)

Comment 2

5 years ago
Comment on attachment 591223 [details] [diff] [review]
Patch

Pushed with a compiling patch :)
Attachment #591223 - Flags: checkin+
(Assignee)

Updated

5 years ago
Flags: in-testsuite-
Target Milestone: --- → mozilla13
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
Target Milestone: mozilla13 → ---
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 :-)
(Assignee)

Comment 5

5 years ago
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 :)
Attachment #605802 - Flags: review?(justin.lebar+bug)
(Assignee)

Updated

5 years ago
Attachment #591223 - Flags: checkin+ → checkin-
(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.
Maybe it's bug 729430 (which I reviewed)?
(Assignee)

Comment 8

5 years ago
Yes, indeed. You did review that bug so you can obviously review this one :)
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?
Attachment #605802 - Flags: review?(justin.lebar+bug) → review-
(Assignee)

Comment 10

5 years ago
Created attachment 605812 [details] [diff] [review]
Update to current tip
Attachment #605802 - Attachment is obsolete: true
Attachment #605812 - Flags: review?(justin.lebar+bug)
> +  nsCOMPtr<nsIDocument> doc = do_GetInterface(GetOwner()->GetDocShell());
> +  nsIPrincipal *principal = doc->NodePrincipal();

doc can't be null?
Attachment #605812 - Flags: review?(justin.lebar+bug) → review+
(Assignee)

Comment 12

5 years ago
(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? :)
(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...
(Assignee)

Comment 14

5 years ago
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.
https://hg.mozilla.org/mozilla-central/rev/38a71ae7e06d
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla14
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
https://hg.mozilla.org/mozilla-central/rev/64c0ad726111
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED

Updated

5 years ago
Depends on: 745495
You need to log in before you can comment on or make changes to this bug.