Closed Bug 1314792 Opened 3 years ago Closed 3 years ago

Extract logic for determining if a DocShell may change process from AttemptLargeAllocationLoad

Categories

(Core :: DOM: Content Processes, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox52 --- fixed

People

(Reporter: Nika, Assigned: Nika)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

nsContentUtils::AttemptLargeAllocationLoad currently makes a series of checks to determine if it would be safe to move the document between content processes. We should extract that logic into a seperate method which can also be used for prerendering.
MozReview-Commit-ID: FI0pIvHPN0h
Attachment #8806915 - Flags: review?(bkelly)
Comment on attachment 8806915 [details] [diff] [review]
Extract process changing logic into nsContentUtils::GetDocShellProcessLock

Review of attachment 8806915 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with comments addressed.

::: dom/base/nsContentUtils.cpp
@@ +9676,5 @@
> +  // If the docshell is not allowed to change process, report an error based on
> +  // the reason
> +  const char* errorName = nullptr;
> +  switch (GetDocShellProcessLock(docShell)) {
> +  case ProcessLockReason::NON_CONTENT_PROC:

The indent seems wrong here.

@@ +9769,5 @@
> +  nsCOMPtr<nsPIDOMWindowOuter> outer = do_GetInterface(aDocShell);
> +
> +  // If we aren't in the content process, we cannot perform a cross-process
> +  // load, so abort right away
> +  if (NS_WARN_IF(!XRE_IsContentProcess())) {

Don't warn for something that we expect to happen under normal conditions and handled in the caller.  Here and below.  Or beware the wrath of :erahm.

::: dom/base/nsContentUtils.h
@@ +2716,5 @@
> +
> +  // This method determines if a given docshell is "Process-Locked", returning
> +  // a ProcessLockReason which is either ProcessLockReason::NONE, which means that
> +  // the docshell is not locked to the current process, or any other value, which
> +  // indicates the reason why the process is locked.

Can you document what "being locked to the process" means?  Based on the bug description I think I get it, but not sure someone just looking at the file would.

Also, "ProcessLock" made me think of some kind of IPC mutex or something.  Again, documentation should help here.

@@ +2717,5 @@
> +  // This method determines if a given docshell is "Process-Locked", returning
> +  // a ProcessLockReason which is either ProcessLockReason::NONE, which means that
> +  // the docshell is not locked to the current process, or any other value, which
> +  // indicates the reason why the process is locked.
> +  static ProcessLockReason GetDocShellProcessLock(nsIDocShell* aDocShell);

If you are returning a ProcessLockReason, can you name this GetDocShellProcessLockReason?

Also, should this just live on nsIDocShell instead of being a static function?
Attachment #8806915 - Flags: review?(bkelly) → review+
Blocks: 1315105
This is an updated version in response to your comments. I'm requesting review again because the changes are decently large and I'd like you to take a quick look at it again.
Attachment #8807597 - Flags: review?(bkelly)
Attachment #8806915 - Attachment is obsolete: true
Comment on attachment 8807597 [details] [diff] [review]
Extract process changing logic into nsContentUtils::GetDocShellProcessLock

Review of attachment 8807597 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!
Attachment #8807597 - Flags: review?(bkelly) → review+
Pushed by michael@thelayzells.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5c1285f9911c
Extract process changing logic into nsContentUtils::GetDocShellProcessLock, r=bkelly
https://hg.mozilla.org/mozilla-central/rev/5c1285f9911c
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in before you can comment on or make changes to this bug.