1336714 - Crash in nsDocument::ComputeFlashClassification

Status: RESOLVED FIXED

(Core :: Plug-ins, defect)

Description

[Marcia Knous [:marcia]]

This bug was filed from the Socorro interface and is 
report bp-9b1b1477-b7a4-43ac-85c3-09e7d2170204.
=============================================================

Seen while looking at nightly crash stats - crashes started using 20170203030204 build: http://bit.ly/2k73Nul

Possible regression range: http://bit.ly/2k73Nul

Possibly triggered by Bug 1323064? ni on Kirk.

Comment 1

[Nicholas Nethercote [inactive]]

#7 topcrash in Windows Nightly 20170203094345, with 73 occurrences.

Comment 2

[Kirk Steuber (he/him) [:bytesized]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8ee95932127d

Comment 3

[Kirk Steuber (he/him) [:bytesized]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=00df8f4e12c6

Comment 4

[Kirk Steuber (he/him) [:bytesized]]

Attachment: Bug 1336714 - Added a null check to the GetParentDocument call in nsDocument::ComputeFlashClassification. Change nsDocument::GetAllowPlugins to return result rather than using unnecessary outparam.

Review commit: https://reviewboard.mozilla.org/r/110208/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/110208/

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8834175 [details]
Bug 1336714 - Added a null check to the GetParentDocument call in nsDocument::ComputeFlashClassification. Change nsDocument::GetAllowPlugins to return result rather than using unnecessary outparam.

https://reviewboard.mozilla.org/r/110208/#review111304

This could use a better commit message describing what we're actually doing: (1) using the document that is actually related to the PluginArray, and (2) adding a null-check to deny if for some reason our document has no parent when it really should.

r=me with that fixed.

Comment 6

[Benjamin Smedberg]

Comment on attachment 8834175 [details]
Bug 1336714 - Added a null check to the GetParentDocument call in nsDocument::ComputeFlashClassification. Change nsDocument::GetAllowPlugins to return result rather than using unnecessary outparam.

https://reviewboard.mozilla.org/r/110208/#review111544

Agree with bz that the commit message should be better: describe what you're changing (both the outparam and the null check).

I defer to bz about whether it's ever expected not to have a parent document here. If it's not actually expected, perhaps an assertion is in order.

I'd like to see this again.

::: dom/base/nsDocument.cpp:3160
(Diff revision 1)
>    if (docShell) {
> -    docShell->GetAllowPlugins(aAllowPlugins);
> +    docShell->GetAllowPlugins(&allowPlugins);
>  
>      // If the docshell allows plugins, we check whether
>      // we are sandboxed and plugins should not be allowed.
> -    if (*aAllowPlugins)
> +    if (allowPlugins)

Now that this doesn't use an outparam, this would be much easier to read as an early return. e.g.

    if (!allowPlugins) {
      return false;
    }
    if (mSandboxFlags & SANDBOXED_PLUGINS) {
      return false;
    }
    if (...classification) {
    
    }
    return true;

Comment 7

[Kirk Steuber (he/him) [:bytesized]]

Comment on attachment 8834175 [details]
Bug 1336714 - Added a null check to the GetParentDocument call in nsDocument::ComputeFlashClassification. Change nsDocument::GetAllowPlugins to return result rather than using unnecessary outparam.

https://reviewboard.mozilla.org/r/110208/#review111544

bz and I discussed this and believe that there is a corner case where the document could be null there.

Comment 8

[Kirk Steuber (he/him) [:bytesized]]

Comment on attachment 8834175 [details]
Bug 1336714 - Added a null check to the GetParentDocument call in nsDocument::ComputeFlashClassification. Change nsDocument::GetAllowPlugins to return result rather than using unnecessary outparam.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/110208/diff/1-2/

Comment 9

[Benjamin Smedberg]

Comment on attachment 8834175 [details]
Bug 1336714 - Added a null check to the GetParentDocument call in nsDocument::ComputeFlashClassification. Change nsDocument::GetAllowPlugins to return result rather than using unnecessary outparam.

https://reviewboard.mozilla.org/r/110208/#review111650

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/cff9ca695cca
Added a null check to the GetParentDocument call in nsDocument::ComputeFlashClassification. Change nsDocument::GetAllowPlugins to return result rather than using unnecessary outparam. nsPluginArray::AllowPlugins changed to call the GetAllowPlugins method on the inner window's document rather than on the docshell's mContentViewer's document. r=bsmedberg,bz

Comment 11

[Carsten Book [:Tomcat]]

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

Comment 12

[Kirk Steuber (he/him) [:bytesized]]

Comment on attachment 8834175 [details]
Bug 1336714 - Added a null check to the GetParentDocument call in nsDocument::ComputeFlashClassification. Change nsDocument::GetAllowPlugins to return result rather than using unnecessary outparam.

Approval Request Comment
[Feature/Bug causing the regression]: This feature is needed for the Shield study to be run on Release 52 (bug 1335232), which we'll use to study the effect of making flash click-to-play by default.
[User impact if declined]: Can't run the study as intended/Uplift of Bug 1307604 will cause crashes.
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: Not for the feature independently. We'll do QE on the study as a whole to make sure all pieces work as expected
[List of other uplifts needed for the feature/fix]: bug 1307604
[Is the change risky?]: No
[Why is the change risky/not risky?]: Simple null check and minor code restructuring.
[String changes made/needed]: none

Comment 13

[Gerry Chang [:gchang]]

Comment on attachment 8834175 [details]
Bug 1336714 - Added a null check to the GetParentDocument call in nsDocument::ComputeFlashClassification. Change nsDocument::GetAllowPlugins to return result rather than using unnecessary outparam.

Fix a crash. Aurora53+.

Comment 14

[Carsten Book [:Tomcat]]

needs rebasing i guess:

merging docshell/base/nsDocShell.cpp
merging dom/base/nsDocument.cpp
merging dom/base/nsDocument.h
merging dom/base/nsIDocument.h
merging dom/base/nsPluginArray.cpp
warning: conflicts while merging dom/base/nsDocument.cpp! (edit, then use 'hg resolve --mark')
abort: unresolved conflicts, can't continue
(use 'hg resolve' and 'hg graft --continue')

Comment 15

[Kirk Steuber (he/him) [:bytesized]]

Ah. I forgot a patch that this also depends on. This should merge successfully after Bug 1307604 and Bug 1323064 have been merged.

Comment 16

[Carsten Book [:Tomcat]]

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

Comment 17

[Julien Cristau [:jcristau]]

Comment on attachment 8834175 [details]
Bug 1336714 - Added a null check to the GetParentDocument call in nsDocument::ComputeFlashClassification. Change nsDocument::GetAllowPlugins to return result rather than using unnecessary outparam.

this was deemed too risky for beta