Closed Bug 827304 Opened 7 years ago Closed 7 years ago

rename browser/base/content/test/browser_clickToPlayPluginScriptAccessPopup.js to reduce the possibility of path-too-long issues

Categories

(Core :: Plug-ins, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla21
Tracking Status
firefox18 --- fixed
firefox19 --- fixed
firefox20 --- fixed
firefox21 --- fixed
firefox-esr17 --- fixed
b2g18 --- fixed

People

(Reporter: bhearsum, Assigned: keeler)

References

Details

Attachments

(2 files, 1 obsolete file)

bug 810082 added a test file with a very long name. This caused us to hit the path length limit during Thunderbird 17.0.2esr release builds. We worked around it by changing the build directory name from something useful to "zzz", but that's not a hack we can keep. Can change this test file name to something a bit more compact to reduce the possibility of hitting this problem?

http://msdn.microsoft.com/en-us/library/aa365247%28VS.85%29.aspx#maxpath has background on the path length limit.
Blocks: 827305
From http://msdn.microsoft.com/en-us/library/aa365247%28VS.85%29.aspx#maxpath:

> The Windows API has many functions that also have Unicode versions to permit 
> an extended-length path for a maximum total path length of 32,767 characters.
...
> To specify an extended-length path, use the "\\?\" prefix.

Is this not an option, or do I not understand the problem? (Or would this require a large effort to change?)

Anyway, I suppose that test could be called something like browser_CTPScriptPlugin.js. This saves 19 characters, it looks like.
(In reply to David Keeler from comment #1)
> From
> http://msdn.microsoft.com/en-us/library/aa365247%28VS.85%29.aspx#maxpath:
> 
> > The Windows API has many functions that also have Unicode versions to permit 
> > an extended-length path for a maximum total path length of 32,767 characters.
> ...
> > To specify an extended-length path, use the "\\?\" prefix.
> 
> Is this not an option, or do I not understand the problem? (Or would this
> require a large effort to change?)

Huh, I'd never noticed that. I guess it depends on whether or not MSYS/Make support that.
Blocks: 827306
(In reply to David Keeler from comment #1)
> From
> http://msdn.microsoft.com/en-us/library/aa365247%28VS.85%29.aspx#maxpath:
> 
> > The Windows API has many functions that also have Unicode versions to permit 
> > an extended-length path for a maximum total path length of 32,767 characters.
> ...
> > To specify an extended-length path, use the "\\?\" prefix.
> 
> Is this not an option, or do I not understand the problem? (Or would this
> require a large effort to change?)
> 
> Anyway, I suppose that test could be called something like
> browser_CTPScriptPlugin.js. This saves 19 characters, it looks like.

Can we do this soon-ish? Our fix to make this caught at original build (or try) can't land until this is fixed and backported everywhere. I'm happy to do the backporting once the original fix is on mozilla-central or inbound.
Attached patch patchSplinter Review
Jared, if you've got a minute, we can probably take care of this pretty quick.
Assignee: nobody → dkeeler
Status: NEW → ASSIGNED
Attachment #703585 - Flags: review?(jaws)
Attachment #703585 - Flags: review?(jaws) → review+
Attached patch patch for other branches (obsolete) — Splinter Review
Not sure if this needs a review or not. The other patch got rejected when I tried to apply it.
Doesn't that patch need the hg rename?

diff --git a/browser/base/content/test/browser_clickToPlayPluginScriptAccessPopup.js b/browser/base/content/test/browser_CTPScriptPlugin.js
rename from browser/base/content/test/browser_clickToPlayPluginScriptAccessPopup.js
rename to browser/base/content/test/browser_CTPScriptPlugin.js
(In reply to David Keeler from comment #7)
> Doesn't that patch need the hg rename?
> 
> diff --git
> a/browser/base/content/test/browser_clickToPlayPluginScriptAccessPopup.js
> b/browser/base/content/test/browser_CTPScriptPlugin.js
> rename from
> browser/base/content/test/browser_clickToPlayPluginScriptAccessPopup.js
> rename to browser/base/content/test/browser_CTPScriptPlugin.js

Indeed! Thanks for catching that.
Attachment #703614 - Attachment is obsolete: true
Attachment #703879 - Flags: review?
Comment on attachment 703879 [details] [diff] [review]
patch for other branches, v2

And it looks like the mozilla-inbound landing went fine, so requesting approval for other branches.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: None
Testing completed: Landed cleanly on mozilla-inbound
Risk to taking this patch (and alternatives if risky): None that I'm aware of (it's a simple file rename)

Drivers, I'm requesting that this patch be backported everywhere because the current name of this file causes us to hit the maximum path length on Windows during certain release builds -- meaning that during a time-sensitive release we could have unforseen build problems. (http://msdn.microsoft.com/en-us/library/aa365247%28VS.85%29.aspx#maxpath). Our longer term "fix" for this is bug 827306, to make sure such problems will be caught at initial landing, but we need this patch first or else we'll burn some trees when we try to land that.
Attachment #703879 - Flags: review?(dkeeler)
Attachment #703879 - Flags: review?
Attachment #703879 - Flags: approval-mozilla-release?
Attachment #703879 - Flags: approval-mozilla-esr17?
Attachment #703879 - Flags: approval-mozilla-beta?
Attachment #703879 - Flags: approval-mozilla-b2g18?
Attachment #703879 - Flags: approval-mozilla-aurora?
Comment on attachment 703879 [details] [diff] [review]
patch for other branches, v2

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

Interesting - it looks like splinter review doesn't handle hg renames.
Anyway, even though I'm not a browser peer, this patch is functionally equivalent to what Jared r+'d, so I think it's fine for me to r+ this. One note would be that this doesn't have the full 8 lines of context we usually require, but since that's probably what caused the first patch to fail to apply, it might not matter (in fact, it would be less work to just make these changes by hand rather than using a patch, because it's likely that this Makefile is different on each branch in this section).
Attachment #703879 - Flags: review?(dkeeler) → review+
Comment on attachment 703879 [details] [diff] [review]
patch for other branches, v2

Approving for all branches given that this appears to have low risk and it's early enough in beta that we could catch any regressions if there were some on building.
Attachment #703879 - Flags: approval-mozilla-release?
Attachment #703879 - Flags: approval-mozilla-release+
Attachment #703879 - Flags: approval-mozilla-esr17?
Attachment #703879 - Flags: approval-mozilla-esr17+
Attachment #703879 - Flags: approval-mozilla-beta?
Attachment #703879 - Flags: approval-mozilla-beta+
Attachment #703879 - Flags: approval-mozilla-b2g18?
Attachment #703879 - Flags: approval-mozilla-b2g18+
Attachment #703879 - Flags: approval-mozilla-aurora?
Attachment #703879 - Flags: approval-mozilla-aurora+
https://hg.mozilla.org/mozilla-central/rev/9511909a46ee
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
https://hg.mozilla.org/releases/mozilla-beta/rev/eaed4086d02b
https://hg.mozilla.org/releases/mozilla-b2g18/rev/2930df78ee77
https://hg.mozilla.org/releases/mozilla-esr17/rev/d1adae6f74b7

This is also in my queue to land on Aurora once it reopens. However, I do not have a clone of mozilla-release handy to land this on, so if someone else can land it there, I would appreciate it.
Thanks for doing all this landing folks. Much appreciated.
You need to log in before you can comment on or make changes to this bug.