Closed
Bug 1336714
Opened 4 years ago
Closed 4 years ago
Crash in nsDocument::ComputeFlashClassification
Categories
(Core :: Plug-ins, defect)
Tracking
()
RESOLVED
FIXED
mozilla54
People
(Reporter: marcia, Assigned: bytesized)
References
(Blocks 1 open bug)
Details
(Keywords: crash)
Crash Data
Attachments
(1 file)
59 bytes,
text/x-review-board-request
|
bzbarsky
:
review+
benjamin
:
review+
gchang
:
approval-mozilla-aurora+
jcristau
:
approval-mozilla-beta-
|
Details |
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)
![]() |
||
Comment 1•4 years ago
|
||
#7 topcrash in Windows Nightly 20170203094345, with 73 occurrences.
Assignee | ||
Comment 2•4 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8ee95932127d
Assignee | ||
Comment 3•4 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=00df8f4e12c6
Comment hidden (mozreview-request) |
Assignee | ||
Updated•4 years ago
|
Assignee: nobody → ksteuber
Flags: needinfo?(ksteuber)
Assignee | ||
Updated•4 years ago
|
Attachment #8834175 -
Flags: review?(bzbarsky)
Assignee | ||
Updated•4 years ago
|
Attachment #8834175 -
Flags: review?(benjamin)
![]() |
||
Comment 5•4 years ago
|
||
mozreview-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/#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 6•4 years ago
|
||
mozreview-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-
Assignee | ||
Comment 7•4 years ago
|
||
mozreview-review-reply |
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.
Assignee | ||
Updated•4 years ago
|
Blocks: flashstudy, 1277346
Comment hidden (mozreview-request) |
Comment 9•4 years ago
|
||
mozreview-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/#review111650
Attachment #8834175 -
Flags: review?(benjamin) → review+
Comment 10•4 years ago
|
||
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
Comment 11•4 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/cff9ca695cca
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Assignee | ||
Comment 12•4 years ago
|
||
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?
Updated•4 years ago
|
Comment 13•4 years ago
|
||
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+
Comment 14•4 years ago
|
||
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)
Assignee | ||
Comment 15•4 years ago
|
||
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 16•4 years ago
|
||
bugherderuplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/f02cc905662a
Comment 17•4 years ago
|
||
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-
Updated•4 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•