Closed Bug 1336714 Opened 3 years ago Closed 3 years ago

Crash in nsDocument::ComputeFlashClassification

Categories

(Core :: Plug-ins, defect, critical)

Unspecified
Windows 10
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox52 --- wontfix
firefox53 --- fixed
firefox54 --- fixed

People

(Reporter: marcia, Assigned: bytesized)

References

(Blocks 1 open bug)

Details

(Keywords: crash)

Crash Data

Attachments

(1 file)

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.
Flags: needinfo?(ksteuber)
#7 topcrash in Windows Nightly 20170203094345, with 73 occurrences.
Assignee: nobody → ksteuber
Flags: needinfo?(ksteuber)
Attachment #8834175 - Flags: review?(bzbarsky)
Attachment #8834175 - Flags: review?(benjamin)
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.
Attachment #8834175 - Flags: review?(bzbarsky) → review+
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;
Attachment #8834175 - Flags: review?(benjamin) → review-
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 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
Attachment #8834175 - Flags: review?(benjamin) → review+
Pushed by ksteuber@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/cff9ca695cca
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
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
Attachment #8834175 - Flags: approval-mozilla-beta?
Attachment #8834175 - Flags: approval-mozilla-aurora?
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+.
Attachment #8834175 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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')
Flags: needinfo?(ksteuber)
Ah. I forgot a patch that this also depends on. This should merge successfully after Bug 1307604 and Bug 1323064 have been merged.
Flags: needinfo?(ksteuber)
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
Attachment #8834175 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
You need to log in before you can comment on or make changes to this bug.